Skip to content

Rustfmt-ing liblog v2. #28991

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

Merged
merged 3 commits into from
Oct 14, 2015
Merged

Rustfmt-ing liblog v2. #28991

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3db5012
rustfmt suggested changes.
Oct 12, 2015
5943af3
Re-running updated rustfmt on liblog.
Oct 13, 2015
94946db
Manually alligned comments.
Oct 13, 2015
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
84 changes: 45 additions & 39 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,48 @@ 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) => {
// if the single argument is a log-level string or number,
// treat that as a global fallback
match parse_log_level(part0) {
Some(num) => (num, None),
None => (::MAX_LOG_LEVEL, Some(part0)),
}
mods.map(|m| {
for s in m.split(',') {
if s.is_empty() {
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
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
match parse_log_level(part0) {
Some(num) => (num, None),
None => (::MAX_LOG_LEVEL, Some(part0)),
}
}
},
_ => {
println!("warning: invalid logging spec '{}', \
ignoring it", s);
continue
}
};
dirs.push(LogDirective {
name: name.map(str::to_owned),
level: log_level,
});
}});
(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,
});
}
});

(dirs, filter.map(str::to_owned))
}
Expand Down
118 changes: 69 additions & 49 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.

Perhaps this could stay as is for now?

Copy link
Member

Choose a reason for hiding this comment

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

This looks OK to me. I don't think it is better than the original, but I wouldn't say its worse. And I'm starting to appreciate having the function names aligned like this.

Copy link
Contributor

Choose a reason for hiding this comment

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

if I were writing this by hand, I think i'd be inclined to insert a newline after the = (though I guess this probably technically fits with whatever rustfmt thinks our max line length is?)

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 @@ -445,16 +444,14 @@ 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
}
];
let dirs = [LogDirective {
name: Some("crate2".to_string()),
level: 3,
},
LogDirective {
name: Some("crate1::mod1".to_string()),
level: 2,
}];
assert!(enabled(2, "crate1::mod1", dirs.iter()));
assert!(!enabled(3, "crate1::mod1", dirs.iter()));
assert!(enabled(3, "crate2", dirs.iter()));
Expand All @@ -463,49 +460,72 @@ 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 }
];
let dirs = [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 }
];
let dirs = [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 }
];
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,
}];
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 }
];
let dirs = [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()));
}

#[test]
fn zero_level() {
let dirs = [
LogDirective { name: None, level: 3 },
LogDirective { name: Some("crate1::mod1".to_string()), level: 0 }
];
let dirs = [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