Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Add conversion between span::Id and NonZeroU64 #770

Merged
merged 1 commit into from
Jun 27, 2020

Conversation

nvzqz
Copy link
Contributor

@nvzqz nvzqz commented Jun 27, 2020

Motivation

Consumers of the crate should be able to handle the 0 case separately and avoiding panicking within Id::from_u64.

Solution

Expose direct conversion between the inner type.

This allows handling the 0 case separately and avoiding panicking within
`Id::from_u64`.
@nvzqz nvzqz requested review from hawkw and a team as code owners June 27, 2020 02:02
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.

looks good to me, thanks!

/// Constructs a new span ID from the given `NonZeroU64`.
///
/// Unlike [`Id::from_u64`](#method.from_u64), this will never panic.
#[inline]
Copy link
Member

Choose a reason for hiding this comment

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

Not convinced we need inline attributes for these --- I can't imagine them ever not being inlined. Not a blocker.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I noticed the lack of #[inline] in this crate. In my experience, not having the attribute results in a function call, which is needless overhead. However, I haven't tested whether this is the case for newer compilers. I remember seeing a compiler issue tracking this behavior.

@hawkw
Copy link
Member

hawkw commented Jun 27, 2020

It occurs to me that these could probably just be From/Into conversions...I don't think they really need to be, since there aren't a whole lot of cases where we really want a duck-type-y API that you can pass a span::Id or a NonZeroU64 into (and in fact, it might not be great to weaken the newtype like that), but it's a thought.

@nvzqz
Copy link
Contributor Author

nvzqz commented Jun 27, 2020

These conversions could definitely (and maybe should) be From conversions. However, u64 conversions I strongly believe should not be, because it's unexpected for a From conversion to fail because of an invalid argument. That's what TryFrom is for.

@hawkw
Copy link
Member

hawkw commented Jun 27, 2020

Yeah, I wasn't thinking we would make the u64 conversion From (although the Into<u64> conversion would be infallible).

Let's merge these with the more explicit name; we can always add From<NonZeroU64>/Into<NonZeroU64> later if anyone needs it.

@hawkw hawkw merged commit 8628f89 into tokio-rs:master Jun 27, 2020
@nvzqz nvzqz deleted the Id-conversions branch June 27, 2020 04:59
hawkw added a commit that referenced this pull request Jul 8, 2020
Changed

- Replaced use of `inner_local_macros` with `$crate::` (#729)

Added

- `must_use` warning to guards returned by `dispatcher::set_default`
  (#686)
- `fmt::Debug` impl to `dyn Value`s (#696)
- Functions to convert between `span::Id` and `NonZeroU64` (#770)
- More obvious warnings in documentation (#769)

Fixed

- Compiler error when `tracing-core/std` feature is enabled but
  `tracing/std` is not (#760)
- Clippy warning on vtable address comparison in `callsite::Identifier`
  (#749)
- Documentation formatting issues (#715, #771)

Thanks to @bkchr, @majecty, @taiki-e, @nagisa, and @nqvz for
contributing to this release!
hawkw added a commit that referenced this pull request Jul 8, 2020
### Changed

- Replaced use of `inner_local_macros` with `$crate::` (#729)

### Added

- `must_use` warning to guards returned by `dispatcher::set_default`
  (#686)
- `fmt::Debug` impl to `dyn Value`s (#696)
- Functions to convert between `span::Id` and `NonZeroU64` (#770)
- More obvious warnings in documentation (#769)

### Fixed

- Compiler error when `tracing-core/std` feature is enabled but
  `tracing/std` is not (#760)
- Clippy warning on vtable address comparison in `callsite::Identifier`
  (#749)
- Documentation formatting issues (#715, #771)

Thanks to @bkchr, @majecty, @taiki-e, @nagisa, and @nvzqz for
contributing to this release!
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants