Skip to content

liblog: add the loglevel, file and linenumber to the log output #13708

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

Closed
wants to merge 1 commit into from
Closed

liblog: add the loglevel, file and linenumber to the log output #13708

wants to merge 1 commit into from

Conversation

Blei
Copy link
Contributor

@Blei Blei commented Apr 23, 2014

No description provided.

@alexcrichton
Copy link
Member

@pcwalton has had concerns in the past about the code bloat of passing around a filename and line number.

I do agree that this is quite useful, but I would expect loggers to only take a level and a format string rather than litany of other relevant arguments. Instead, perhaps the macro could change similar to this?

macro_rules! log_with_location(
    ($lvl:expr, $fmt $($arg:tt)*) => ({
        log!($lvl, concat!("{} {}:{}", $fmt), ::log::level_to_str($lvl), file!(), line!() $($arg)*)
    })
)

@seanmonstar
Copy link
Contributor

I'd actually think a Logger would rather receive a LogRecord of some such, that includes the message, the name (from directive), level, and a few other possible properties. Loggers could want to handle different messages differently (send db errors as an email).

@Blei
Copy link
Contributor Author

Blei commented Apr 24, 2014

I agree that code bloat could potentially be a problem, not sure to what extent 2 static arguments make much of a difference. I don't agree with the proposed solution though. What's the point of making custom loggers possible, if the majority of the formatting is already prescribed? If the two additional arguments are the main concern, then gathering them into a struct as proposed by @seanmonstar would be a nicer solution IMO.

@alexcrichton
Copy link
Member

In that case, I like the suggestion by @seanmonstar as well, it'd also allow some nice extensibility in the future?

Would you be ok with making that change @Blei? Also, you may want to print just the filename of the paths because sometimes the path to the file can be quite long.

@alexcrichton
Copy link
Member

(sorry it took me a bit to get back to this!)

@seanmonstar
Copy link
Contributor

@alexcrichton I can make a PR with what I had in mind. One question: how would I use the fmt::Arguments provided in a new format string? I can use fmt::format(args), but I don't want to allocate a string just to feed back into a new format string.

fn log(record: &LogRecord) {
    write!(stdout, "{} {}: {}", record.file, record.line, record.args);
}

@alexcrichton
Copy link
Member

Something along the lines of:

fn log(record: &LogRecord) -> IoResult<()> {
    try!(write!(stdout, "{} {}: ", record.file, record.line));
    fmt::write(stdout, record.args)
}

@seanmonstar
Copy link
Contributor

Ouch. Would it make sense to add a Show implementation to Arguments in the same patch, to allow the example I used?

@alexcrichton
Copy link
Member

Sure!

@alexcrichton
Copy link
Member

Closing in favor of #13912

@hirschenberger
Copy link
Contributor

@seanmonstar Was it intentional using write! instead of writeln!?

@seanmonstar
Copy link
Contributor

Nope, that was a mistake. Woops!

flip1995 pushed a commit to flip1995/rust that referenced this pull request Nov 28, 2024
Suggested by, and closes rust-lang#13704.

changelog: [`unnecessary_map_or`]: use a clearer lint message
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.

4 participants