Skip to content

Commit

Permalink
core: make Level and LevelFilter Copy (#992)
Browse files Browse the repository at this point in the history
## Motivation

This makes both structs easier to use because you no longer have to
worry about borrow errors while working with them. There's no downside
to making them `Copy` since both are wrappers around a `usize`.

Usually, we try to avoid adding `Copy` implementations for public API
types, as it presents a forward-compatibility hazard in the event that
the internal representation of those types must change to one that is no
longer trivially copyable. However, in this case, we rely on `Level` and 
`LevelFilter` having a `usize` representation (and there's a test 
asserting that `LevelFilter`s can be transmuted to `usize`). Any change
to the internal representation would already be breaking, so it's okay to
commit to `Copy` here.

Ideally, this would make `Metadata::level` return `Level` instead of
`&Level`. However that's a breaking change, so I didn't make it here.

## Solution

Derive `Copy` for both structs and fix various warnings that popped up
as a result.

Since this changes crates that depend on tracing-core to remove clone
calls and rely on Level and LevelFilter being Copy, those crates will no
longer compile with older versions of tracing-core. This bumps the
version number for `tracing-core` and sets the minimum version to the
new version for all affected crates. This avoids compile errors with a
cached `Cargo.lock`.
  • Loading branch information
jyn514 authored Sep 28, 2020
1 parent a8cc977 commit 8d6ebf1
Show file tree
Hide file tree
Showing 12 changed files with 31 additions and 31 deletions.
2 changes: 1 addition & 1 deletion tracing-core/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ name = "tracing-core"
# - README.md
# - Update CHANGELOG.md.
# - Create "v0.1.x" git tag.
version = "0.1.16"
version = "0.1.17"
authors = ["Tokio Contributors <team@tokio.rs>"]
license = "MIT"
readme = "README.md"
Expand Down
8 changes: 4 additions & 4 deletions tracing-core/src/metadata.rs
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ pub struct Metadata<'a> {
pub struct Kind(KindInner);

/// Describes the level of verbosity of a span or event.
#[derive(Clone, Debug, PartialEq, Eq)]
#[derive(Copy, Clone, Debug, PartialEq, Eq)]
pub struct Level(LevelInner);

/// A filter comparable to a verbosity `Level`.
Expand All @@ -107,7 +107,7 @@ pub struct Level(LevelInner);
/// addition of an `OFF` level that completely disables all trace
/// instrumentation.
#[repr(transparent)]
#[derive(Clone, Eq, PartialEq)]
#[derive(Copy, Clone, Eq, PartialEq)]
pub struct LevelFilter(Option<Level>);

/// Indicates that a string could not be parsed to a valid level.
Expand Down Expand Up @@ -858,13 +858,13 @@ mod tests {
(LevelFilter::DEBUG, LevelInner::Debug as usize),
(LevelFilter::TRACE, LevelInner::Trace as usize),
];
for &(ref filter, expected) in &mapping {
for &(filter, expected) in &mapping {
let repr = unsafe {
// safety: The entire purpose of this test is to assert that the
// actual repr matches what we expect it to be --- we're testing
// that *other* unsafe code is sound using the transmuted value.
// We're not going to do anything with it that might be unsound.
mem::transmute::<_, usize>(filter.clone())
mem::transmute::<LevelFilter, usize>(filter)
};
assert_eq!(expected, repr, "repr changed for {:?}", filter)
}
Expand Down
2 changes: 1 addition & 1 deletion tracing-log/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ log-tracer = []
trace-logger = []

[dependencies]
tracing-core = { path = "../tracing-core", version = "0.1.2"}
tracing-core = { path = "../tracing-core", version = "0.1.17"}
log = { version = "0.4" }
lazy_static = "1.3.0"
env_logger = { version = "0.7", optional = true }
Expand Down
10 changes: 5 additions & 5 deletions tracing-log/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -310,8 +310,8 @@ lazy_static! {
static ref ERROR_FIELDS: Fields = Fields::new(ERROR_CS);
}

fn level_to_cs(level: &Level) -> (&'static dyn Callsite, &'static Fields) {
match *level {
fn level_to_cs(level: Level) -> (&'static dyn Callsite, &'static Fields) {
match level {
Level::TRACE => (TRACE_CS, &*TRACE_FIELDS),
Level::DEBUG => (DEBUG_CS, &*DEBUG_FIELDS),
Level::INFO => (INFO_CS, &*INFO_FIELDS),
Expand Down Expand Up @@ -415,13 +415,13 @@ impl<'a> NormalizeEvent<'a> for Event<'a> {
fn normalized_metadata(&'a self) -> Option<Metadata<'a>> {
let original = self.metadata();
if self.is_log() {
let mut fields = LogVisitor::new_for(self, level_to_cs(original.level()).1);
let mut fields = LogVisitor::new_for(self, level_to_cs(*original.level()).1);
self.record(&mut fields);

Some(Metadata::new(
"log event",
fields.target.unwrap_or("log"),
original.level().clone(),
*original.level(),
fields.file,
fields.line.map(|l| l as u32),
fields.module_path,
Expand All @@ -434,7 +434,7 @@ impl<'a> NormalizeEvent<'a> for Event<'a> {
}

fn is_log(&self) -> bool {
self.metadata().callsite() == identify_callsite!(level_to_cs(self.metadata().level()).0)
self.metadata().callsite() == identify_callsite!(level_to_cs(*self.metadata().level()).0)
}
}

Expand Down
2 changes: 1 addition & 1 deletion tracing-log/tests/log_tracer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ impl Subscriber for TestSubscriber {
event.normalized_metadata().map(|normalized| OwnedMetadata {
name: normalized.name().to_string(),
target: normalized.target().to_string(),
level: normalized.level().clone(),
level: *normalized.level(),
module_path: normalized.module_path().map(String::from),
file: normalized.file().map(String::from),
line: normalized.line(),
Expand Down
2 changes: 1 addition & 1 deletion tracing-subscriber/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ registry = ["sharded-slab", "thread_local"]
json = ["tracing-serde", "serde", "serde_json"]

[dependencies]
tracing-core = { path = "../tracing-core", version = "0.1.16" }
tracing-core = { path = "../tracing-core", version = "0.1.17" }

# only required by the filter feature
matchers = { optional = true, version = "0.0.1" }
Expand Down
18 changes: 9 additions & 9 deletions tracing-subscriber/src/filter/env/directive.rs
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ impl Directive {
Some(StaticDirective {
target: self.target.clone(),
field_names,
level: self.level.clone(),
level: self.level,
})
}

Expand Down Expand Up @@ -119,7 +119,7 @@ impl Directive {
.ok()?;
Some(field::CallsiteMatch {
fields,
level: self.level.clone(),
level: self.level,
})
}

Expand Down Expand Up @@ -417,9 +417,9 @@ impl<T: Match + Ord> DirectiveSet<T> {
pub(crate) fn add(&mut self, directive: T) {
// does this directive enable a more verbose level than the current
// max? if so, update the max level.
let level = directive.level();
if *level > self.max_level {
self.max_level = level.clone();
let level = *directive.level();
if level > self.max_level {
self.max_level = level;
}
// insert the directive into the vec of directives, ordered by
// specificity (length of target + number of field filters). this
Expand Down Expand Up @@ -460,8 +460,8 @@ impl Dynamics {
return Some(f);
}
match base_level {
Some(ref b) if d.level > *b => base_level = Some(d.level.clone()),
None => base_level = Some(d.level.clone()),
Some(ref b) if d.level > *b => base_level = Some(d.level),
None => base_level = Some(d.level),
_ => {}
}
None
Expand Down Expand Up @@ -690,7 +690,7 @@ impl CallsiteMatcher {
.collect();
SpanMatcher {
field_matches,
base_level: self.base_level.clone(),
base_level: self.base_level,
}
}
}
Expand All @@ -702,7 +702,7 @@ impl SpanMatcher {
.iter()
.filter_map(field::SpanMatch::filter)
.max()
.unwrap_or_else(|| self.base_level.clone())
.unwrap_or(self.base_level)
}

pub(crate) fn record_update(&self, record: &span::Record<'_>) {
Expand Down
4 changes: 2 additions & 2 deletions tracing-subscriber/src/filter/env/field.rs
Original file line number Diff line number Diff line change
Expand Up @@ -229,7 +229,7 @@ impl CallsiteMatch {
.collect();
SpanMatch {
fields,
level: self.level.clone(),
level: self.level,
has_matched: AtomicBool::new(false),
}
}
Expand Down Expand Up @@ -263,7 +263,7 @@ impl SpanMatch {
#[inline]
pub(crate) fn filter(&self) -> Option<LevelFilter> {
if self.is_matched() {
Some(self.level.clone())
Some(self.level)
} else {
None
}
Expand Down
2 changes: 1 addition & 1 deletion tracing/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ keywords = ["logging", "tracing", "metrics", "async"]
edition = "2018"

[dependencies]
tracing-core = { path = "../tracing-core", version = "0.1.15", default-features = false }
tracing-core = { path = "../tracing-core", version = "0.1.17", default-features = false }
log = { version = "0.4", optional = true }
tracing-attributes = { path = "../tracing-attributes", version = "0.1.10", optional = true }
cfg-if = "0.1.10"
Expand Down
4 changes: 2 additions & 2 deletions tracing/src/macros.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2150,7 +2150,7 @@ macro_rules! fieldset {
#[macro_export]
macro_rules! level_to_log {
($level:expr) => {
match *$level {
match $level {
$crate::Level::ERROR => $crate::log::Level::Error,
$crate::Level::WARN => $crate::log::Level::Warn,
$crate::Level::INFO => $crate::log::Level::Info,
Expand Down Expand Up @@ -2297,7 +2297,7 @@ macro_rules! __tracing_log {
(target: $target:expr, $level:expr, $($field:tt)+ ) => {
$crate::if_log_enabled! {{
use $crate::log;
let level = $crate::level_to_log!(&$level);
let level = $crate::level_to_log!($level);
if level <= log::STATIC_MAX_LEVEL && level <= log::max_level() {
let log_meta = log::Metadata::builder()
.level(level)
Expand Down
6 changes: 3 additions & 3 deletions tracing/src/span.rs
Original file line number Diff line number Diff line change
Expand Up @@ -552,7 +552,7 @@ impl Span {
} else {
meta.target()
};
span.log(target, level_to_log!(meta.level()), format_args!("++ {}{}", meta.name(), FmtAttrs(attrs)));
span.log(target, level_to_log!(*meta.level()), format_args!("++ {}{}", meta.name(), FmtAttrs(attrs)));
}}

span
Expand Down Expand Up @@ -925,7 +925,7 @@ impl Span {
} else {
meta.target()
};
self.log(target, level_to_log!(meta.level()), format_args!("{}{}", meta.name(), FmtValues(&record)));
self.log(target, level_to_log!(*meta.level()), format_args!("{}{}", meta.name(), FmtValues(&record)));
}
}}

Expand Down Expand Up @@ -1028,7 +1028,7 @@ impl Span {
#[inline]
fn log(&self, target: &str, level: log::Level, message: fmt::Arguments<'_>) {
if let Some(ref meta) = self.meta {
if level_to_log!(meta.level()) <= log::max_level() {
if level_to_log!(*meta.level()) <= log::max_level() {
let logger = log::logger();
let log_meta = log::Metadata::builder().level(level).target(target).build();
if logger.enabled(&log_meta) {
Expand Down
2 changes: 1 addition & 1 deletion tracing/tests/support/subscriber.rs
Original file line number Diff line number Diff line change
Expand Up @@ -200,7 +200,7 @@ where
}
}
fn max_level_hint(&self) -> Option<LevelFilter> {
self.max_level.clone()
self.max_level
}

fn record(&self, id: &Id, values: &span::Record<'_>) {
Expand Down

0 comments on commit 8d6ebf1

Please sign in to comment.