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

improve newtype_index! macro #50337

Closed
nikomatsakis opened this issue Apr 30, 2018 · 5 comments
Closed

improve newtype_index! macro #50337

nikomatsakis opened this issue Apr 30, 2018 · 5 comments
Assignees
Labels
C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@nikomatsakis
Copy link
Contributor

nikomatsakis commented Apr 30, 2018

We have this handy macro newtype_index! that creates a, well, newtype'd index:

newtype_index!(RegionAtLocationIndex { DEBUG_FORMAT = "RegionAtLocationIndex({})" });

However, I think it could be improved in four ways:

First, the syntax should change to include the keyword struct and pub. This would help with people trying to find the definition of the type. I'd probably switch to {} form at the same time, but that's not necessary:

newtype_index! {
  pub struct RegionAtLocationIndex { DEBUG_FORMAT = "RegionAtLocationIndex({})" }
}

Second, doing this would also allow us to support arbitrary visibilities. For example, I'd like to make crate struct RegionAtLocationIndex:

newtype_index! {
  crate struct RegionAtLocationIndex { DEBUG_FORMAT = "RegionAtLocationIndex({})" }
}

Third, we should change to incorporate NonZero. That is, the original example would currently expands to something like:

pub struct RegionAtLocationIndex(u32);

but I want it to expand to:

pub struct RegionAtLocationIndex { non_zero: NonZero<u32> }

Of course, 0 is a valid index, so the various initialization and accessor routines would have to add or subtract one as appropriate.

Using NonZero would mean that Option<T> would be represented still as a single u32.

Finally, fourth, as a convenience, it would be nice to define inherent (non-trait) methods, so that using these types did not require importing the Idx trait:

        impl $type {
            #[inline]
            fn new(value: usize) -> Self {..}
            #[inline]
            fn index(self) -> usize {..}
        }

cc @Nashenas88 @spastorino @sgrif

@nikomatsakis nikomatsakis added C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Apr 30, 2018
@spastorino
Copy link
Member

I can tackle this!

@spastorino spastorino self-assigned this Apr 30, 2018
@sgrif
Copy link
Contributor

sgrif commented Apr 30, 2018

FWIW I'd prefer we do pub(crate) struct instead of crate struct, so we continue to map to the corresponding Rust syntax. We should be able to just the vis matcher

@petrochenkov
Copy link
Contributor

newtype_index! macro

Oh, that thing stopping "Go To Definition" from working 😄

@nikomatsakis
Copy link
Contributor Author

nikomatsakis commented Apr 30, 2018

@sgrif

FWIW I'd prefer we do pub(crate) struct instead of crate struct, so we continue to map to the corresponding Rust syntax. We should be able to just the vis matcher

crate is Rust syntax =) -- the vis matcher would be fine

@sgrif
Copy link
Contributor

sgrif commented Apr 30, 2018

Oh neat, I was not aware of #45388

kennytm added a commit to kennytm/rust that referenced this issue Sep 8, 2018
…Simulacrum

use `NonZeroU32` in `newtype_index!`macro, change syntax

Various improvements to the `newtype_index!` macro:

- Use `NonZeroU32` so that `Option<T>` is cheap
- More ergonomic helper method, no need to import `Idx` trait all the time
- Improve syntax to use `struct` keyword so that ripgrep works to find type def'n

Fixes rust-lang#50337

I'm curious to see if this passes tests =)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

4 participants