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.

Sign up for GitHub

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

Rustfmt-ing liblog. #28898

Closed
wants to merge 1 commit into from
Closed

Rustfmt-ing liblog. #28898

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
76 changes: 39 additions & 37 deletions src/liblog/directive.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,15 +17,17 @@ pub struct LogDirective {
pub level: u32,
}

pub const LOG_LEVEL_NAMES: [&'static str; 4] = ["ERROR", "WARN", "INFO",
"DEBUG"];
pub const LOG_LEVEL_NAMES: [&'static str; 4] = ["ERROR", "WARN", "INFO", "DEBUG"];

/// Parse an individual log level that is either a number or a symbolic log level
fn parse_log_level(level: &str) -> Option<u32> {
level.parse::<u32>().ok().or_else(|| {
let pos = LOG_LEVEL_NAMES.iter().position(|&name| name.eq_ignore_ascii_case(level));
pos.map(|p| p as u32 + 1)
}).map(|p| cmp::min(p, ::MAX_LOG_LEVEL))
level.parse::<u32>()
.ok()
.or_else(|| {
let pos = LOG_LEVEL_NAMES.iter().position(|&name| name.eq_ignore_ascii_case(level));
pos.map(|p| p as u32 + 1)
})
.map(|p| cmp::min(p, ::MAX_LOG_LEVEL))
}

/// Parse a logging specification string (e.g: "crate1,crate2::mod3,crate3::x=1/foo")
Expand All @@ -40,44 +42,44 @@ pub fn parse_logging_spec(spec: &str) -> (Vec<LogDirective>, Option<String>) {
let mods = parts.next();
let filter = parts.next();
if parts.next().is_some() {
println!("warning: invalid logging spec '{}', \
ignoring it (too many '/'s)", spec);
println!("warning: invalid logging spec '{}', ignoring it (too many '/'s)", spec);
return (dirs, None);
}
mods.map(|m| { for s in m.split(',') {
if s.is_empty() { continue }
let mut parts = s.split('=');
let (log_level, name) = match (parts.next(), parts.next().map(|s| s.trim()), parts.next()) {
(Some(part0), None, None) => {
mods.map(|m| {
for s in m.split(',') {
if s.is_empty() {
continue
}
Copy link
Member

Choose a reason for hiding this comment

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

I think that there's a bug to leave this one alone, can't remember exactly though.

Copy link
Member

Choose a reason for hiding this comment

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

I think this is OK (apart from a possible ; on the continue. I filed something about one line if expressions, but that would only be in expression position in any case.

let mut parts = s.split('=');
let (log_level, name) = match (parts.next(),
parts.next().map(|s| s.trim()),
parts.next()) {
(Some(part0), None, None) => {
// if the single argument is a log-level string or number,
// treat that as a global fallback
Copy link
Member

Choose a reason for hiding this comment

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

Could you manually align these comments please? (There is already a rustfmt bug filed for this).

match parse_log_level(part0) {
Some(num) => (num, None),
None => (::MAX_LOG_LEVEL, Some(part0)),
match parse_log_level(part0) {
Some(num) => (num, None),
None => (::MAX_LOG_LEVEL, Some(part0)),
}
}
}
(Some(part0), Some(""), None) => (::MAX_LOG_LEVEL, Some(part0)),
(Some(part0), Some(part1), None) => {
match parse_log_level(part1) {
Some(num) => (num, Some(part0)),
_ => {
println!("warning: invalid logging spec '{}', \
ignoring it", part1);
continue
(Some(part0), Some(""), None) => (::MAX_LOG_LEVEL, Some(part0)),
(Some(part0), Some(part1), None) => {
match parse_log_level(part1) {
Some(num) => (num, Some(part0)),
_ => {
println!("warning: invalid logging spec '{}', ignoring it", part1);
continue
}
}
}
},
_ => {
println!("warning: invalid logging spec '{}', \
ignoring it", s);
continue
}
};
dirs.push(LogDirective {
name: name.map(str::to_owned),
level: log_level,
});
}});
_ => {
println!("warning: invalid logging spec '{}', ignoring it", s);
continue
}
};
dirs.push(LogDirective { name: name.map(str::to_owned), level: log_level, });
Copy link
Member

Choose a reason for hiding this comment

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

hmm, there shouldn't be a trailing comma here.

Copy link
Member

Choose a reason for hiding this comment

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

https://github.com/nrc/rustfmt/issues/421

}
});

(dirs, filter.map(str::to_owned))
}
Expand Down
68 changes: 30 additions & 38 deletions src/liblog/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -235,7 +235,9 @@ pub trait Logger {
fn log(&mut self, record: &LogRecord);
}

struct DefaultLogger { handle: Stderr }
struct DefaultLogger {
handle: Stderr,
}

/// Wraps the log level with fmt implementations.
#[derive(Copy, Clone, PartialEq, PartialOrd, Debug)]
Expand All @@ -246,7 +248,7 @@ impl fmt::Display for LogLevel {
let LogLevel(level) = *self;
match LOG_LEVEL_NAMES.get(level as usize - 1) {
Some(ref name) => fmt::Display::fmt(name, fmt),
None => fmt::Display::fmt(&level, fmt)
None => fmt::Display::fmt(&level, fmt),
}
}
}
Expand Down Expand Up @@ -301,11 +303,10 @@ pub fn log(level: u32, loc: &'static LogLocation, args: fmt::Arguments) {
// Completely remove the local logger from TLS in case anyone attempts to
// frob the slot while we're doing the logging. This will destroy any logger
// set during logging.
let mut logger: Box<Logger + Send> = LOCAL_LOGGER.with(|s| {
s.borrow_mut().take()
}).unwrap_or_else(|| {
box DefaultLogger { handle: io::stderr() }
});
let mut logger: Box<Logger + Send> = LOCAL_LOGGER.with(|s| s.borrow_mut().take())
.unwrap_or_else(|| {
box DefaultLogger { handle: io::stderr() }
});
Copy link
Member

Choose a reason for hiding this comment

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

Hm this is a bit of an interesting pattern, @nrc I at least find my self doing something like this pretty frequently (e.g. here and here, although I may just be weird!

I personally find the previous formatting more readable than the latter at least

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, I'd be interested to see what rustfmt makes of those larger blocks. I bet it's awkward formatting them with visual indent like this.

I used to prefer the original formatting, but the new one has grown on me (it was kind of unintended, but having the higher order functions aligned makes things very readable for me). It could be a difference between large and small blocks, which would be sad, because that is the kind of thing rustfmt finds hard to differentiate.

Copy link
Member

Choose a reason for hiding this comment

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

Ah yeah that's a good point about the size of the blocks. The benefit I've seen from not aligning-on-period is that indentation works a little better inside a multi-line closure. For example if you always indent 4 spaces beyond the period that may be some non-multiple-of-4 spaces from the left-hand side, which sometimes causes editors confusion (or people!)

logger.log(&LogRecord {
level: LogLevel(level),
args: args,
Expand All @@ -320,22 +321,21 @@ pub fn log(level: u32, loc: &'static LogLocation, args: fmt::Arguments) {
/// safely
#[doc(hidden)]
#[inline(always)]
pub fn log_level() -> u32 { unsafe { LOG_LEVEL } }
pub fn log_level() -> u32 {
unsafe { LOG_LEVEL }
}

/// Replaces the thread-local logger with the specified logger, returning the old
/// logger.
pub fn set_logger(logger: Box<Logger + Send>) -> Option<Box<Logger + Send>> {
let mut l = Some(logger);
LOCAL_LOGGER.with(|slot| {
mem::replace(&mut *slot.borrow_mut(), l.take())
})
LOCAL_LOGGER.with(|slot| mem::replace(&mut *slot.borrow_mut(), l.take()))
}

/// A LogRecord is created by the logging macros, and passed as the only
/// argument to Loggers.
#[derive(Debug)]
pub struct LogRecord<'a> {

/// The module path of where the LogRecord originated.
pub module_path: &'a str,

Expand Down Expand Up @@ -373,7 +373,9 @@ pub fn mod_enabled(level: u32, module: &str) -> bool {
// again to whether they should really be here or not. Hence, despite this
// check being expanded manually in the logging macro, this function checks
// the log level again.
if level > unsafe { LOG_LEVEL } { return false }
if level > unsafe { LOG_LEVEL } {
return false
}

// This assertion should never get tripped unless we're in an at_exit
// handler after logging has been torn down and a logging attempt was made.
Expand All @@ -385,14 +387,11 @@ pub fn mod_enabled(level: u32, module: &str) -> bool {
}
}

fn enabled(level: u32,
module: &str,
iter: slice::Iter<directive::LogDirective>)
-> bool {
fn enabled(level: u32, module: &str, iter: slice::Iter<directive::LogDirective>) -> bool {
// Search for the longest match, the vector is assumed to be pre-sorted.
for directive in iter.rev() {
match directive.name {
Some(ref name) if !module.starts_with(&name[..]) => {},
Some(ref name) if !module.starts_with(&name[..]) => {}
Some(..) | None => {
return level <= directive.level
}
Expand Down Expand Up @@ -446,14 +445,8 @@ mod tests {
#[test]
fn match_full_path() {
let dirs = [
LogDirective {
name: Some("crate2".to_string()),
level: 3
},
LogDirective {
name: Some("crate1::mod1".to_string()),
level: 2
}
LogDirective {name: Some("crate2".to_string()), level: 3, },
LogDirective {name: Some("crate1::mod1".to_string()), level: 2, }
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if the lack of a space in front of name is a bug in rustdoc? cc @nrc

Copy link
Member

Choose a reason for hiding this comment

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

rustfmt? Yeah, could be.

];
assert!(enabled(2, "crate1::mod1", dirs.iter()));
assert!(!enabled(3, "crate1::mod1", dirs.iter()));
Expand All @@ -464,37 +457,36 @@ mod tests {
#[test]
fn no_match() {
let dirs = [
LogDirective { name: Some("crate2".to_string()), level: 3 },
LogDirective { name: Some("crate1::mod1".to_string()), level: 2 }
LogDirective { name: Some("crate2".to_string()), level: 3, },
LogDirective { name: Some("crate1::mod1".to_string()), level: 2, }
];
assert!(!enabled(2, "crate3", dirs.iter()));
}

#[test]
fn match_beginning() {
let dirs = [
LogDirective { name: Some("crate2".to_string()), level: 3 },
LogDirective { name: Some("crate1::mod1".to_string()), level: 2 }
LogDirective { name: Some("crate2".to_string()), level: 3, },
LogDirective { name: Some("crate1::mod1".to_string()), level: 2, }
];
assert!(enabled(3, "crate2::mod1", dirs.iter()));
}

#[test]
fn match_beginning_longest_match() {
let dirs = [
LogDirective { name: Some("crate2".to_string()), level: 3 },
LogDirective { name: Some("crate2::mod".to_string()), level: 4 },
LogDirective { name: Some("crate1::mod1".to_string()), level: 2 }
];
LogDirective { name: Some("crate2".to_string()), level: 3, },
LogDirective { name: Some("crate2::mod".to_string()), level: 4, },
LogDirective { name: Some("crate1::mod1".to_string()), level: 2, }];
assert!(enabled(4, "crate2::mod1", dirs.iter()));
assert!(!enabled(4, "crate2", dirs.iter()));
}

#[test]
fn match_default() {
let dirs = [
LogDirective { name: None, level: 3 },
LogDirective { name: Some("crate1::mod1".to_string()), level: 2 }
LogDirective { name: None, level: 3, },
LogDirective { name: Some("crate1::mod1".to_string()), level: 2, }
];
assert!(enabled(2, "crate1::mod1", dirs.iter()));
assert!(enabled(3, "crate2::mod2", dirs.iter()));
Expand All @@ -503,8 +495,8 @@ mod tests {
#[test]
fn zero_level() {
let dirs = [
LogDirective { name: None, level: 3 },
LogDirective { name: Some("crate1::mod1".to_string()), level: 0 }
LogDirective { name: None, level: 3, },
LogDirective { name: Some("crate1::mod1".to_string()), level: 0, }
];
assert!(!enabled(1, "crate1::mod1", dirs.iter()));
assert!(enabled(3, "crate2::mod2", dirs.iter()));
Expand Down