Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

properly handle the off pseudo-level in presence of nested logging directives #605

Merged
merged 2 commits into from
Oct 13, 2021

Conversation

japaric
Copy link
Member

@japaric japaric commented Oct 6, 2021

this handles nested logging directives like:

$ # NOTE the order of these directives doesn't matter
$ DEFMT_LOG=krate=info,krate::module=off,krate::module::inner=info

by treating the off pseudo-level as a explicit "reject" criteria and checking logging directives
from the innermost one (the one that has more module path segments) to the outermost ones

manually tested with:

fn main() -> ! {
    defmt::info!("main");
    bar::bar();
    bar::inner::inner();

    app::exit()
}

mod bar {
    pub mod inner {
        pub fn inner() {
            defmt::info!("inner");
        }
    }

    pub fn bar() {
        defmt::info!("bar");
    }
}
$ DEFMT_LOG=hello=info,hello::bar=off cargo rb hello
INFO  main

$ DEFMT_LOG=hello=info,hello::bar=off,hello::bar::inner=info cargo rb hello
INFO  main
INFO  inner

…directives

this handles nested logging directives like:
```
$ # NOTE the order of these directives doesn't matter
$ DEFMT_LOG=krate=info,krate::module=off,krate::module::inner=info
```

by treating the `off` pseudo-level as a explicit "reject" criteria and checking logging directives
from the innermost one (the one that has more module path segments) to the outermost ones
Copy link
Member

@Urhengulas Urhengulas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. But I didn't fully understand the the code and mainly checked if the tests make sense.

And I added one question and one change request :)

// `module_path!` returns the path to the module the function is in, not the path to the function
// itself
fn codegen_is_inside_of_check(parent_module_path: &str) -> TokenStream2 {
let parent = parent_module_path.as_bytes();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we compare the module names on the byte-level and not as strings?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

at the end we want to do str::starts_with in const context but that method cannot be used in const context so instead we do a byte by byte equality check (see the macro expansion). we cannot index into a str and get a u8 so instead we use [u8] slices

}

pub(crate) fn as_str(&self) -> &str {
&self.0
pub(super) fn to_str(&self) -> String {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For consistency with std, this should read .to_string.

Suggested change
pub(super) fn to_str(&self) -> String {
pub(super) fn to_string(&self) -> String {

@japaric
Copy link
Member Author

japaric commented Oct 13, 2021

bors r=Urhengulas

@bors
Copy link
Contributor

bors bot commented Oct 13, 2021

Build succeeded:

@bors bors bot merged commit 91a0cb9 into main Oct 13, 2021
@bors bors bot deleted the defmt-log-nested-off branch October 13, 2021 14:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants