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

Make Level and LevelFilter Copy #992

Merged
merged 3 commits into from
Sep 28, 2020
Merged

Conversation

jyn514
Copy link
Contributor

@jyn514 jyn514 commented Sep 26, 2020

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.

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.

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`.

Ideally, this would make `Metadata::Level` return `Level` instead of
`&Level`. However that's a breaking change, so I didn't make it here.
@jyn514 jyn514 requested review from hawkw and a team as code owners September 26, 2020 23:33
@jyn514
Copy link
Contributor Author

jyn514 commented Sep 26, 2020

This is split out from #990 (see #990 (comment)).

@jyn514
Copy link
Contributor Author

jyn514 commented Sep 26, 2020

I don't know how to fix this error :(

 ---- metadata::tests::level_filter_reprs stdout ----
thread 'metadata::tests::level_filter_reprs' panicked at 'assertion failed: `(left == right)`
  left: `5`,
 right: `140704621814760`: repr changed for LevelFilter::OFF', tracing-core/src/metadata.rs:869:13
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

@jyn514
Copy link
Contributor Author

jyn514 commented Sep 26, 2020

I tried this but it didn't work:

diff --git a/tracing-core/src/metadata.rs b/tracing-core/src/metadata.rs
index 8dd216f..d9ea459 100644
--- a/tracing-core/src/metadata.rs
+++ b/tracing-core/src/metadata.rs
@@ -432,7 +432,7 @@ impl LevelFilter {
     // `LevelFilter`. If this is the case, converting a `usize` value into a
     // `LevelFilter` (in `LevelFilter::current`) will be an identity conversion,
     // rather than generating a lookup table.
-    const OFF_USIZE: usize = LevelInner::Error as usize + 1;
+    const OFF_USIZE: usize = unsafe { core::mem::transmute(LevelFilter(None)) };
 
     /// Returns a `LevelFilter` that matches the most verbose [`Level`] that any
     /// currently active [`Subscriber`] will enable.
@@ -851,7 +851,7 @@ mod tests {
     #[test]
     fn level_filter_reprs() {
         let mapping = [
-            (LevelFilter::OFF, LevelInner::Error as usize + 1),
+            (LevelFilter::OFF, LevelFilter::OFF_USIZE),
             (LevelFilter::ERROR, LevelInner::Error as usize),
             (LevelFilter::WARN, LevelInner::Warn as usize),
             (LevelFilter::INFO, LevelInner::Info as usize),
thread 'metadata::tests::level_filter_reprs' panicked at 'assertion failed: `(left == right)`
  left: `5`,
 right: `140381994394536`: repr changed for LevelFilter::OFF', tracing-core/src/metadata.rs:869:13

It was comparing the pointers, not the levels themselves.
This also changes transmute to explicitly specify both types to make
this easier to debug in the future.
@hawkw
Copy link
Member

hawkw commented Sep 27, 2020

Historically, we've avoided adding Copy implementations for public API types, because this represents a potential breaking change if it ever becomes necessary to change the type's internal representation to one that is no longer trivially copyable. In this case, I don't think that's terribly likely, both it's worth weighing the cost before deciding whether to move forward with committing to Copy implementations for these types.

@jyn514
Copy link
Contributor Author

jyn514 commented Sep 27, 2020

Given that there's an explicit test for transmuting between usize and LevelFilter, I think it makes sense to add the Copy bound. Breaking Copy would also break the transmute.

@hawkw
Copy link
Member

hawkw commented Sep 27, 2020

That's fair, in this particular case it's probably justified! :)

Copy link
Member

@hawkw hawkw left a comment

Choose a reason for hiding this comment

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

Since this PR 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. Can we bump the tracing-core version and update tracing, tracing-log, and tracing-subscriber to depend on the new tracing-core version as part of this PR? Thanks!

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`.
@jyn514
Copy link
Contributor Author

jyn514 commented Sep 27, 2020

Good idea, done!

Copy link
Member

@hawkw hawkw left a comment

Choose a reason for hiding this comment

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

Okay, this looks good to me.

in re:

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

We can make this change in tracing-core 0.2 --- we'll add it to the list of planned breaking changes in #922

@hawkw hawkw merged commit 8d6ebf1 into tokio-rs:master Sep 28, 2020
@jyn514 jyn514 deleted the copy-not-clone branch September 28, 2020 16:07
hawkw added a commit that referenced this pull request Sep 28, 2020
Fixed

- Incorrect inlining of `Event::dispatch` and `Event::child_of`, which
  could result in `dispatcher::get_default` being inlined at the
  callsite (#994)

Added

- `Copy` implementations for `Level` and `LevelFilter` (#992)

Thanks to new contributors @jyn514 and @TaKO8Ki for contributing to this
release!

Signed-off-by: Eliza Weisman <eliza@buoyant.io>
hawkw added a commit that referenced this pull request Sep 28, 2020
### Fixed

- Incorrect inlining of `Event::dispatch` and `Event::child_of`, which
  could result in `dispatcher::get_default` being inlined at the
  callsite (#994)

### Added

- `Copy` implementations for `Level` and `LevelFilter` (#992)

Thanks to new contributors @jyn514 and @TaKO8Ki for contributing 
to this release!

Signed-off-by: Eliza Weisman <eliza@buoyant.io>
hawkw added a commit that referenced this pull request Sep 28, 2020
Fixed

- Incorrect inlining of `Span::new`, `Span::new_root`, and
  `Span::new_child_of`, which could result in `dispatcher::get_default`
  being  inlined at the callsite ([#994])
- Regression where using a struct field as a span or event field when
  other fields on that struct are borrowed mutably would fail to compile
  ([#987])

Changed

- Updated `tracing-core` to 0.1.17 ([#992])

Added

- `Instrument` trait and `Instrumented` type for attaching a `Span` to a
  `Future` ([#808])
- `Copy` implementations for `Level` and `LevelFilter` ([#992])
- Multiple documentation fixes and improvements ([#964], [#980], [#981])

Thanks to @nagisa, and new contributors @securityinsanity, @froydnj,
@jyn514 and @TaKO8Ki for contributing to this release!

[#994]: #994
[#992]: #992
[#987]: #987
[#980]: #980
[#981]: #981
[#964]: #964
[#808]: #808

Signed-off-by: Eliza Weisman <eliza@buoyant.io>
hawkw added a commit that referenced this pull request Sep 28, 2020
### Fixed

- Incorrect inlining of `Span::new`, `Span::new_root`, and
  `Span::new_child_of`, which could result in `dispatcher::get_default`
  being  inlined at the callsite ([#994])
- Regression where using a struct field as a span or event field when
  other fields on that struct are borrowed mutably would fail to compile
  ([#987])

### Changed

- Updated `tracing-core` to 0.1.17 ([#992])

### Added

- `Instrument` trait and `Instrumented` type for attaching a `Span` to a
  `Future` ([#808])
- `Copy` implementations for `Level` and `LevelFilter` ([#992])
- Multiple documentation fixes and improvements ([#964], [#980], [#981])

Thanks to @nagisa, and new contributors @securityinsanity, @froydnj,
@jyn514 and @TaKO8Ki for contributing to this release!

[#994]: #994
[#992]: #992
[#987]: #987
[#980]: #980
[#981]: #981
[#964]: #964
[#808]: #808

Signed-off-by: Eliza Weisman <eliza@buoyant.io>
dvdplm added a commit to dvdplm/tracing that referenced this pull request Oct 7, 2020
* upstream/master:
  subscriber: warn if trying to enable a statically disabled level (tokio-rs#990)
  subscriber: use macros for module declarations (tokio-rs#1009)
  chore: remove `stdlib.rs` (tokio-rs#1008)
  core: fix linked list tests reusing `Registration`s (tokio-rs#1016)
  subscriber: support dash in target names (tokio-rs#1012)
  docs: switch to intra-doc links in tracing-core (tokio-rs#1010)
  tracing-opentelemetry: implement additional record types (bool, i64, u64) (tokio-rs#1007)
  core: add intrusive linked list for callsite registry (tokio-rs#988)
  serde: allow tracing-serde to work on no_std. (tokio-rs#960)
  tracing: remove `Into<Option<Id>>` impl for `Span` (tokio-rs#1003)
  tracing: make `Entered` `!Send` (tokio-rs#1001)
  chore: fix nightly clippy warnings (tokio-rs#991)
  chore: bump all crate versions (tokio-rs#998)
  macros: fix the `tracing-macros` crate not compiling (tokio-rs#1000)
  tracing: prepare to release 0.1.21 (tokio-rs#997)
  core: prepare to release 0.1.17 (tokio-rs#996)
  subscriber: make `PartialOrd` & `Ord` impls more correct (tokio-rs#995)
  core, tracing: don't inline dispatcher::get_default (tokio-rs#994)
  core: make `Level` and `LevelFilter` `Copy` (tokio-rs#992)
hawkw added a commit that referenced this pull request Oct 7, 2020
Changed

- Updated `tracing-core` to 0.1.17 ([#992])

Added

- **env-filter**: Added support for filtering on targets which contain
  dashes ([#1014])
- **env-filter**: Added a warning when creating an `EnvFilter` that
  contains directives that would enable a level disabled by the
  `tracing` crate's `static_max_level` features ([#1021])

Thanks to @jyn514 and @bkchr for contributing to this release!

[#992]: #992
[#1014]: #1014
[#1021]: #1021
hawkw added a commit that referenced this pull request Oct 7, 2020
Changed

- Updated `tracing-core` to 0.1.17 ([#992])

Added

- **env-filter**: Added support for filtering on targets which contain
  dashes ([#1014])
- **env-filter**: Added a warning when creating an `EnvFilter` that
  contains directives that would enable a level disabled by the
  `tracing` crate's `static_max_level` features ([#1021])

Thanks to @jyn514 and @bkchr for contributing to this release!

[#992]: #992
[#1014]: #1014
[#1021]: #1021
hawkw added a commit that referenced this pull request Oct 7, 2020
### Changed

- Updated `tracing-core` to 0.1.17 ([#992])

### Added

- **env-filter**: Added support for filtering on targets which contain
  dashes ([#1014])
- **env-filter**: Added a warning when creating an `EnvFilter` that
  contains directives that would enable a level disabled by the
  `tracing` crate's `static_max_level` features ([#1021])

Thanks to @jyn514 and @bkchr for contributing to this release!

[#992]: #992
[#1014]: #1014
[#1021]: #1021
kaffarell pushed a commit to kaffarell/tracing that referenced this pull request May 22, 2024
### Changed

- Updated `tracing-core` to 0.1.17 ([tokio-rs#992])

### Added

- **env-filter**: Added support for filtering on targets which contain
  dashes ([tokio-rs#1014])
- **env-filter**: Added a warning when creating an `EnvFilter` that
  contains directives that would enable a level disabled by the
  `tracing` crate's `static_max_level` features ([tokio-rs#1021])

Thanks to @jyn514 and @bkchr for contributing to this release!

[tokio-rs#992]: tokio-rs#992
[tokio-rs#1014]: tokio-rs#1014
[tokio-rs#1021]: tokio-rs#1021
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