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

Remove outdated doc that no longer holds true #261

Merged
merged 1 commit into from
Jan 18, 2019

Conversation

king6cong
Copy link
Contributor

@king6cong king6cong commented Jan 9, 2019

debug! are removed by default

old: https://github.com/rust-lang/rust/blob/90346eae18e83887517e096c17678a74838ff995/src/liblog/macros.rs#L163-L166
new: https://github.com/rust-lang-nursery/log/blob/433731637477b27f573a3e40cd2297c5a776ff1b/src/macros.rs#L136-L144

I can't find conditional compilation that will remove debug!, only find runtime cfg!(debug_assertions) check in the old liblog, maybe I need to dig into the more distant history.

expressions within the debug! call are run whenever RUST_LOG is set, even if the filter would exclude the log

https://github.com/rust-lang-nursery/log/blob/433731637477b27f573a3e40cd2297c5a776ff1b/src/macros.rs#L36-L41

It seems RUST_LOG's presence alone does not make a difference, it's the value that matters

expressions are evaluated even if logging is not enabled in your module, they are not formatted unless it is.

I think if expressions are evaluated, they are certain to be formatted, which makes this unnecessary?

I am not sure about these, please correct me if I'm wrong 😄

Centril added a commit to Centril/rust that referenced this pull request Jan 12, 2019
@mark-i-m
Copy link
Member

Hmm... thanks @king6cong! It does indeed appear that the docs need to be updated, but I'm not really knowledgeable enough to say how.

It seems that one still needs debug-assertions enabled to get debug! output. I don't know how this happens, but I would guess somehow the build system uses something like this: https://docs.rs/log/0.4.6/log/index.html#compile-time-filters...

In particular, I would rather update the doc than simply delete obsolete parts. Would you be interested in digging into this a bit more?

@king6cong
Copy link
Contributor Author

king6cong commented Jan 17, 2019

@mark-i-m Thanks for the insight 😄

https://github.com/rust-lang/rust/blob/master/src/librustc/Cargo.toml#L19
https://github.com/rust-lang-nursery/log/blob/433731637477b27f573a3e40cd2297c5a776ff1b/src/lib.rs#L1271-L1272
https://github.com/rust-lang-nursery/log/blob/433731637477b27f573a3e40cd2297c5a776ff1b/src/macros.rs#L36

  1. with debug_assertions being true
  2. with feature-flag release_max_level_info enabled
  3. const comparison in log! macro

These three will remove debug! and trace! related code from the final binary. I will update the docs

@king6cong king6cong force-pushed the master branch 2 times, most recently from eb1e813 to 62cd91d Compare January 17, 2019 16:24
@king6cong
Copy link
Contributor Author

@mark-i-m
Updated
Some change:

  • remove information related to rust-forge, which doesn't have the corresponding page now: Removed debuggin.md after moving it to rustc-guide rust-forge#134
  • add information about the log crate and put it together with env-logger
  • add section about keeping or removing debug! and trace! calls from the resulting binary
  • remove information about putting expensive/crashy operations inside an fmt::Debug impl, which is not needed because if expressions are evaluated, it will be formatted.

Copy link
Member

@mark-i-m mark-i-m left a comment

Choose a reason for hiding this comment

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

Overall, looks good to me!

I left one minor comment, and I had the following question:

remove information about putting expensive/crashy operations inside an fmt::Debug impl, which is not needed because if expressions are evaluated, it will be formatted.

Sorry, how did you come to this conclusion? Does debug! only evaluate expressions if debug logging is enabled?

### How to keep or remove `debug!` and `trace!` calls from the resulting binary

While calls to `error!`, `warn!` and `info!` are included in every build of the compiler,
calls to `debug!` and `trace!` are only included in the program if the
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
calls to `debug!` and `trace!` are only included in the program if the
calls to `debug!` and `trace!` are only included in the program if

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated

@king6cong
Copy link
Contributor Author

king6cong commented Jan 18, 2019

Sorry, how did you come to this conclusion? Does debug! only evaluate expressions if debug logging is enabled?

expressions will be evaluate and fmt will be called if debug logging is enabled. These two come together, both or neither.

related code here:

https://github.com/rust-lang-nursery/log/blob/433731637477b27f573a3e40cd2297c5a776ff1b/src/macros.rs#L37-L41

also verified:

#[macro_use]
extern crate log;
extern crate env_logger;

use std::fmt;

struct ExpensiveOperationContainer {}

impl ExpensiveOperationContainer {
    fn new() -> ExpensiveOperationContainer {
        println!("ExpensiveOperationContainer::new called");
        ExpensiveOperationContainer {}
    }
}

impl fmt::Debug for ExpensiveOperationContainer {
    fn fmt(&self, fmt: &mut fmt::Formatter) -> fmt::Result {
        println!("Debug::fmt called");
        fmt::Debug::fmt("", fmt)
    }
}

fn main() {
    env_logger::init();
    warn!("{:?}", ExpensiveOperationContainer::new());
    error!("{:?}", ExpensiveOperationContainer::new());
}

Run with warn enabled: RUST_LOG=warn ./target/release/rt

ExpensiveOperationContainer::new called
Debug::fmt called
 WARN 2019-01-18T07:45:16Z: rt: ""
ExpensiveOperationContainer::new called
Debug::fmt called
ERROR 2019-01-18T07:45:16Z: rt: ""

Run with warn disabled: RUST_LOG=error ./target/release/rt,

ExpensiveOperationContainer::new called
Debug::fmt called
ERROR 2019-01-18T07:45:13Z: rt: ""

We can see if logging is enabled, ExpensiveOperationContainer::new and Debug::fmt called both will be called. If logging is disabled, these won't be called

@mark-i-m
Copy link
Member

Thanks @king6cong for investigating! This looks good to me :)

@mark-i-m mark-i-m merged commit be67227 into rust-lang:master Jan 18, 2019
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