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

Generic unique identifiers. #1106

Closed
wants to merge 1 commit into from
Closed

Generic unique identifiers. #1106

wants to merge 1 commit into from

Conversation

losfair
Copy link
Contributor

@losfair losfair commented Dec 23, 2019

This PR adds a generic unique identifier implementation that can be used for e.g. middleware InternalFields and backend IDs.

TODO:

  • Make it work on stable (macros?)
  • Tests

Copy link
Contributor

@MarkMcCaskey MarkMcCaskey left a comment

Choose a reason for hiding this comment

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

Sorry to hit you with so many requests for changes that aren't directly related to what you're doing, but we really need to improve our doc comments in general!

pub const fn allocate() -> UniqueId<B> {
UniqueId {
init: Once::new(),
inner: UnsafeCell::new(::std::usize::MAX),
Copy link
Contributor

Choose a reason for hiding this comment

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

std::usize::max is fine in modern Rust; usize::max_value() is also fine (doesn't need to be qualified at all)

pub struct UniqueId<B: UniqueIdBacking> {
/// Init once field.
init: Once,
/// Inner field.
Copy link
Contributor

Choose a reason for hiding this comment

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

While we're touching this code, can be do a bit of clean up here and explain what inner is and why it's in an UnsafeCell?

/// A unique identifier.
pub struct UniqueId<B: UniqueIdBacking> {
/// Init once field.
init: Once,
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd like to update this, too -- the doc comment doesn't say anything the source code doesn't already say

init: Once,
/// Inner field.
inner: UnsafeCell<usize>,
_phantom: PhantomData<B>,
Copy link
Contributor

Choose a reason for hiding this comment

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

And if you have context on this part of the code, I'd appreciate a description of the phantom data and the entire thing struct in general -- if you don't understand the code then don't worry about it


fn get_counter() -> &'static ::std::sync::atomic::AtomicUsize {
static COUNTER: ::std::sync::atomic::AtomicUsize =
::std::sync::atomic::AtomicUsize::new(0);
Copy link
Contributor

Choose a reason for hiding this comment

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

We can remove the leading :: now and consider importing std::sync::atomic or at least sync so this can be sync::atomic::AtomicUsize or atomic::AtomicUsize

@@ -0,0 +1,78 @@
//! Unique identifiers.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd appreciate more of a description here too, about exactly what it is and why we'd use it. But this is the lowest priority of the doc comments I'm asking for

@syrusakbary
Copy link
Member

Thanks to our work in the refactor and new internal structures, this PR is no longer needed.
Closing it

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.

3 participants