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

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

None yet

2 participants