Skip to content

Improve newtype_index macro to handle description and constants consistently #45110

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.

Sign up for GitHub

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

Merged
bors merged 3 commits into from
Oct 13, 2017
Merged

Improve newtype_index macro to handle description and constants consistently #45110

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
35545b3
Improve newtype_index macro to handle description and constants consi…
Nashenas88 Oct 8, 2017
9423bee
Move const qualifier from brackets to constant values and remove comm…
Nashenas88 Oct 12, 2017
97fe353
Split lines longer than 100 columns
Nashenas88 Oct 12, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 13 additions & 8 deletions src/librustc/mir/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -402,9 +402,11 @@ pub enum BorrowKind {
///////////////////////////////////////////////////////////////////////////
// Variables and temps

newtype_index!(Local, "_");

pub const RETURN_POINTER: Local = Local(0);
newtype_index!(Local,
const {
Copy link
Contributor Author

@Nashenas88 Nashenas88 Oct 8, 2017

Choose a reason for hiding this comment

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

@nikomatsakis this design is based on a conversation I had with @spastorino. So MAX and DESCRIPTION are handled as special cases, but all other items in this block are constant. Did I interpret this correctly or was there some other nuance I missed?

DESCRIPTION = "_",
Copy link
Contributor

@nikomatsakis nikomatsakis Oct 9, 2017

Choose a reason for hiding this comment

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

Nit: I don't think I would include DESCRIPTION in the list of constants, since it's not a constant.. But I sort of like having one big list. Maybe we should just remove the const keyword and instead write something like:

newtype_index!(Local {
    DESCRIPTION = "_",
    RETURN_POINTER = 0,
});

Alternatively, put the const keyword on the actual constants:

newtype_index!(Local {
    DESCRIPTION = "_",
    const RETURN_POINTER = 0,
});

Copy link
Member

@spastorino spastorino Oct 9, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

And we could even lowercase the non constants ...

newtype_index!(Local {
    debug_name = "_",
    const RETURN_POINTER = 0,
});

Copy link
Contributor Author

@Nashenas88 Nashenas88 Oct 10, 2017

Choose a reason for hiding this comment

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

If we go with something like

newtype_index!(Local {
    DESCRIPTION = "_",
    const RETURN_POINTER = 0,
});

Can we add the restriction that MAX and DEBUG_NAME come first? Coming up with a macro that supports an optional, initial keyword for each definition in the list is going to be much more challenging. It may not be worth the extra effort. I can document that limitation at the macro definition.

@spastorino I would still keep them all caps since their values don't change. I think the const keyword will signify that it's an externally accessible definition.

Copy link
Contributor Author

@Nashenas88 Nashenas88 Oct 10, 2017

Choose a reason for hiding this comment

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

For clarification, the challenge comes in defining the macro expression to parse the entire list. Something like

{ $($(const)* $idents:ident = $constants:expr),+ }

Fails with local ambiguity: multiple parsing options: built-in NTs ident ('idents') or 1 other option..
I can change it to $($definitions:tt),* but then I'll need to account for macro variable type casting (things like @as_expr $e:expr and @as_ident $i:ident) to convert the generic tokens into something more parselable. I'm not sure how long that will take to figure out vs just distinguishing between non-consts first followed by consts.

Copy link
Contributor

@nikomatsakis nikomatsakis Oct 10, 2017

Choose a reason for hiding this comment

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

@Nashenas88

Can we add the restriction that MAX and DEBUG_NAME come first?

Definitely. This is an internal macro, so it doesn't have to be particularly flexible. I'd say make it so that MAX / DEBUG_NAME must come first and in a particular order.

(Incidentally, usually when I have to struct macros like this, I have a kind of "consume loop" where it peels off tokens from the front and then recursively calls itself on the remaining tokens. But I guess that's what you were trying to avoid.)

Copy link
Contributor Author

@Nashenas88 Nashenas88 Oct 12, 2017

Choose a reason for hiding this comment

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

Somehow I missed this comment and in the meantime implemented a version that does work and is less lines. It doesn't enforce MAX and DEBUG_NAME coming first, though. Should I still add that restriction in?

RETURN_POINTER = 0,
});

/// Classifies locals into categories. See `Mir::local_kind`.
#[derive(PartialEq, Eq, Debug)]
Expand Down Expand Up @@ -538,7 +540,7 @@ pub struct UpvarDecl {
///////////////////////////////////////////////////////////////////////////
// BasicBlock

newtype_index!(BasicBlock, "bb");
newtype_index!(BasicBlock, const { DESCRIPTION = "bb" });

///////////////////////////////////////////////////////////////////////////
// BasicBlockData and Terminator
Expand Down Expand Up @@ -1118,7 +1120,7 @@ pub type LvalueProjection<'tcx> = Projection<'tcx, Lvalue<'tcx>, Local, Ty<'tcx>
/// and the index is a local.
pub type LvalueElem<'tcx> = ProjectionElem<'tcx, Local, Ty<'tcx>>;

newtype_index!(Field, "field");
newtype_index!(Field, const { DESCRIPTION = "field" });

impl<'tcx> Lvalue<'tcx> {
pub fn field(self, f: Field, ty: Ty<'tcx>) -> Lvalue<'tcx> {
Expand Down Expand Up @@ -1183,8 +1185,11 @@ impl<'tcx> Debug for Lvalue<'tcx> {
///////////////////////////////////////////////////////////////////////////
// Scopes

newtype_index!(VisibilityScope, "scope");
pub const ARGUMENT_VISIBILITY_SCOPE : VisibilityScope = VisibilityScope(0);
newtype_index!(VisibilityScope,
const {
DESCRIPTION = "scope",
ARGUMENT_VISIBILITY_SCOPE = 0,
});

#[derive(Clone, Debug, RustcEncodable, RustcDecodable)]
pub struct VisibilityScopeData {
Expand Down Expand Up @@ -1509,7 +1514,7 @@ pub struct Constant<'tcx> {
pub literal: Literal<'tcx>,
}

newtype_index!(Promoted, "promoted");
newtype_index!(Promoted, const { DESCRIPTION = "promoted" });

#[derive(Clone, PartialEq, Eq, Hash, RustcEncodable, RustcDecodable)]
pub enum Literal<'tcx> {
Expand Down
87 changes: 67 additions & 20 deletions src/librustc_data_structures/indexed_vec.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,39 +40,86 @@ impl Idx for u32 {

#[macro_export]
macro_rules! newtype_index {
($name:ident) => (
newtype_index!($name, unsafe { ::std::intrinsics::type_name::<$name>() });
);
// ---- private rules ----

($name:ident, $debug_name:expr) => (
// Base case, user-defined constants (if any) have already been defined
(@type[$type:ident] @max[$max:expr] @descr[$descr:expr]) => (
#[derive(Copy, Clone, PartialEq, Eq, Hash, PartialOrd, Ord,
RustcEncodable, RustcDecodable)]
pub struct $name(u32);

impl $name {
// HACK use for constants
#[allow(unused)]
const fn const_new(x: u32) -> Self {
$name(x)
}
}
RustcEncodable, RustcDecodable)]
pub struct $type(u32);

impl Idx for $name {
impl Idx for $type {
fn new(value: usize) -> Self {
assert!(value < (::std::u32::MAX) as usize);
$name(value as u32)
assert!(value < ($max) as usize);
$type(value as u32)
}
fn index(self) -> usize {
self.0 as usize
}
}

impl ::std::fmt::Debug for $name {
impl ::std::fmt::Debug for $type {
fn fmt(&self, fmt: &mut ::std::fmt::Formatter) -> ::std::fmt::Result {
write!(fmt, "{}{}", $debug_name, self.0)
write!(fmt, "{}{}", $descr, self.0)
}
}
)
);

// Replace existing default for max (as final param)
(@type[$type:ident] @max[$_max:expr] @descr[$descr:expr] MAX = $max:expr) => (
newtype_index!(@type[$type] @max[$max] @descr[$descr]);
);

// Replace existing default for max
(@type[$type:ident] @max[$_max:expr] @descr[$descr:expr] MAX = $max:expr, $($idents:ident = $constants:expr),*) => (
newtype_index!(@type[$type] @max[$max] @descr[$descr]);
);

// Replace existing default for description (as final param)
(@type[$type:ident] @max[$max:expr] @descr[$_descr:expr] DESCRIPTION = $descr:expr) => (
newtype_index!(@type[$type] @max[$max] @descr[$descr]);
);

// Replace existing default for description
(@type[$type:ident] @max[$max:expr] @descr[$_descr:expr] DESCRIPTION = $descr:expr, $($idents:ident = $constants:expr),*) => (
newtype_index!(@type[$type] @max[$max] @descr[$descr] $($idents = $constants),*);
);

// Assign a user-defined constant (as final param)
(@type[$type:ident] @max[$max:expr] @descr[$descr:expr] $name:ident = $constant:expr) => (
pub const $name: $type = $type($constant);
Copy link
Contributor Author

@Nashenas88 Nashenas88 Oct 8, 2017

Choose a reason for hiding this comment

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

I'm making all consts pub here, this is how all instances were used. Does anyone think this should be configurable? Maybe something like

newtype_index!(NewType,
    const {
        pub RETURN_LOCATION = 0, // this one is public
        PRIVATE_LOCATION = 1, // this one is private
    });

Copy link
Contributor

@nikomatsakis nikomatsakis Oct 9, 2017

Choose a reason for hiding this comment

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

I don't see a need for private constants, personally. We can always change it later I guess.

newtype_index!(@type[$type] @max[$max] @descr[$descr]);
);

// Assign a user-defined constant
(@type[$type:ident] @max[$max:expr] @descr[$descr:expr] $name:ident = $constant:expr, $($idents:ident = $constants:expr),*) => (
pub const $name: $type = $type($constant);
newtype_index!(@type[$type] @max[$max] @descr[$descr] $($idents = $constants),*);
);

// ---- public rules ----

// Use default constants
($name:ident) => (
newtype_index!(
@type[$name]
@max[::std::u32::MAX]
@descr[unsafe {::std::intrinsics::type_name::<$name>() }]);
);

// Define any constants
($name:ident, const { $($idents:ident = $constants:expr,)+ }) => (
newtype_index!(
@type[$name]
@max[::std::u32::MAX]
@descr[unsafe {::std::intrinsics::type_name::<$name>() }]
$($idents = $constants),+);
);

// Rewrite missing trailing comma in const to version with trailing comma
($name:ident, const { $($idents:ident = $constants:expr),+ }) => (
Copy link
Contributor

@nikomatsakis nikomatsakis Oct 9, 2017

Choose a reason for hiding this comment

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

Nit: I would prefer to start with the PUBLIC rules. I always find it easier to read top-down -- that is, start from the public starting point, then gradually come to the details.

newtype_index!($name, const { $($idents = $constants,)+ });
);
}

#[derive(Clone, PartialEq, Eq)]
Expand Down