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

chore: backport "fix deprecation and clippy warnings" #1208

Merged
merged 1 commit into from
Jan 26, 2021

Conversation

davidbarsky
Copy link
Member

Backports #1195 to v0.1.x; unblocks #1205 and #1207.

This branch fixes two known warnings.

The first fixed warning was in `tracing-core/src/callsite.rs`, where
clippy suggested using `std::ptr::eq` to compare addresses instead of
casting `&T` to a `*const T`, which `&T` coerces to _anyways_. Below is
the warning:

```
error: use `std::ptr::eq` when comparing raw pointers
   --> tracing-core/src/callsite.rs:238:9
    |
238 |         self.0 as *const _ as *const () == other.0 as *const _ as *const ()
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `std::ptr::eq(self.0 as *const _, other.0 as *const _)`
    |
    = note: `-D clippy::ptr-eq` implied by `-D warnings`
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#ptr_eq
```

The second fixed warning is a bit more complex. As of Rust 1.50,
[`AtomicUsize::compare_and_swap`][1] has been deprecated in favor of
[`AtomicUsize::compare_exchange`][2] and
[`AtomicUsize::compare_exchange_weak`][3]. I've moved
`tracing_core::dispatch::set_global_default` to use
`AtomicUsize::compare_exchange` as `AtomicUsize::compare_exchange_weak`
is designed for atomic loads in loops. Here are a few notes on the
differences between `AtomicUsize::compare_and_swap` and
`AtomicUsize::compare_exchange`:
- `AtomicUsize::compare_exchange` returns a result, where the
  `Result::Ok(_)` branch contains the prior value. This is equivalent
  to the now-deprecated [`AtomicUsize::compare_and_swap`][1] returning
  the `current` parameter upon a successful compare-and-swap operation,
  but success is now established through a `Result`, not an equality
  operation.
- `AtomicUsize::compare_exchange` accepts an `Ordering` for the failure
  case of a compare-and-swap. The [migration guide suggests][4] using
  `Ordering::SeqCst` for both the success and failure cases and I saw
  no reason to depart from its guidance.

After this branch is approved, I'll backport these fixes to the `v0.1.0` branch.

[1]: https://doc.rust-lang.org/nightly/std/sync/atomic/struct.AtomicUsize.html#method.compare_and_swap
[2]: https://doc.rust-lang.org/nightly/std/sync/atomic/struct.AtomicUsize.html#method.compare_exchange
[3]: https://doc.rust-lang.org/nightly/std/sync/atomic/struct.AtomicUsize.html#method.compare_exchange_weak
[4]: https://doc.rust-lang.org/nightly/std/sync/atomic/struct.AtomicUsize.html#migrating-to-compare_exchange-and-compare_exchange_weak
@davidbarsky davidbarsky requested a review from a team as a code owner January 26, 2021 15:01
Copy link
Member

@tobz tobz left a comment

Choose a reason for hiding this comment

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

LGTM!

@davidbarsky davidbarsky merged commit 05efda1 into v0.1.x Jan 26, 2021
@davidbarsky davidbarsky deleted the davidbarsky/backport-fix-clippy-warnings branch January 26, 2021 15:48
kaffarell pushed a commit to kaffarell/tracing that referenced this pull request May 22, 2024
…1208)

This branch fixes two known warnings.

The first fixed warning was in `tracing-core/src/callsite.rs`, where
clippy suggested using `std::ptr::eq` to compare addresses instead of
casting `&T` to a `*const T`, which `&T` coerces to _anyways_. Below is
the warning:

```
error: use `std::ptr::eq` when comparing raw pointers
   --> tracing-core/src/callsite.rs:238:9
    |
238 |         self.0 as *const _ as *const () == other.0 as *const _ as *const ()
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `std::ptr::eq(self.0 as *const _, other.0 as *const _)`
    |
    = note: `-D clippy::ptr-eq` implied by `-D warnings`
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#ptr_eq
```

The second fixed warning is a bit more complex. As of Rust 1.50,
[`AtomicUsize::compare_and_swap`][1] has been deprecated in favor of
[`AtomicUsize::compare_exchange`][2] and
[`AtomicUsize::compare_exchange_weak`][3]. I've moved
`tracing_core::dispatch::set_global_default` to use
`AtomicUsize::compare_exchange` as `AtomicUsize::compare_exchange_weak`
is designed for atomic loads in loops. Here are a few notes on the
differences between `AtomicUsize::compare_and_swap` and
`AtomicUsize::compare_exchange`:
- `AtomicUsize::compare_exchange` returns a result, where the
  `Result::Ok(_)` branch contains the prior value. This is equivalent
  to the now-deprecated [`AtomicUsize::compare_and_swap`][1] returning
  the `current` parameter upon a successful compare-and-swap operation,
  but success is now established through a `Result`, not an equality
  operation.
- `AtomicUsize::compare_exchange` accepts an `Ordering` for the failure
  case of a compare-and-swap. The [migration guide suggests][4] using
  `Ordering::SeqCst` for both the success and failure cases and I saw
  no reason to depart from its guidance.

After this branch is approved, I'll backport these fixes to the `v0.1.0` branch.

[1]: https://doc.rust-lang.org/nightly/std/sync/atomic/struct.AtomicUsize.html#method.compare_and_swap
[2]: https://doc.rust-lang.org/nightly/std/sync/atomic/struct.AtomicUsize.html#method.compare_exchange
[3]: https://doc.rust-lang.org/nightly/std/sync/atomic/struct.AtomicUsize.html#method.compare_exchange_weak
[4]: https://doc.rust-lang.org/nightly/std/sync/atomic/struct.AtomicUsize.html#migrating-to-compare_exchange-and-compare_exchange_weak
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