Improve newtype_index macro to handle description and constants consistently #45110

Nashenas88 · 2017-10-08T19:26:18Z

No description provided.

…stently

rust-highfive · 2017-10-08T19:26:24Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @eddyb (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

Nashenas88 · 2017-10-08T19:26:29Z

r? @nikomatsakis

Nashenas88 · 2017-10-08T19:28:18Z

src/librustc_data_structures/indexed_vec.rs

+
+    // 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);


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 });

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

Nashenas88 · 2017-10-08T19:29:56Z

src/librustc/mir/mod.rs

-
-pub const RETURN_POINTER: Local = Local(0);
+newtype_index!(Local,
+    const {


@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?

spastorino · 2017-10-08T23:28:14Z

I'd rename DESCRIPTION to DEBUG_NAME

nikomatsakis

This is great. =) I left a few nits, but I like this overall direction.

nikomatsakis · 2017-10-09T13:08:30Z

src/librustc/mir/mod.rs

-pub const RETURN_POINTER: Local = Local(0);
+newtype_index!(Local,
+    const {
+        DESCRIPTION = "_",


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, });

And we could even lowercase the non constants ...

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

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.

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.

@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.)

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?

nikomatsakis · 2017-10-09T13:09:36Z

src/librustc_data_structures/indexed_vec.rs

+
+    // 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);


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

nikomatsakis · 2017-10-09T13:10:21Z

src/librustc_data_structures/indexed_vec.rs

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


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.

…a from after identifier

arielb1 · 2017-10-12T11:42:41Z

@bors r+

bors · 2017-10-12T11:42:41Z

📌 Commit 97fe353 has been approved by arielb1

Mark-Simulacrum · 2017-10-13T14:39:31Z

@bors rollup

nikomatsakis · 2017-10-13T15:25:24Z

src/librustc/mir/mod.rs

+newtype_index!(VisibilityScope
+    {
+        DEBUG_NAME = "scope",
+        const ARGUMENT_VISIBILITY_SCOPE = 0,


very cool =)

Improve newtype_index macro to handle description and constants consistently

Rollup of 14 pull requests - Successful merges: #44855, #45110, #45122, #45133, #45173, #45178, #45189, #45203, #45209, #45221, #45236, #45240, #45245, #45253 - Failed merges:

Improve newtype_index macro to handle description and constants consi…

35545b3

…stently

rust-highfive assigned eddyb Oct 8, 2017

rust-highfive assigned nikomatsakis and unassigned eddyb Oct 8, 2017

Nashenas88 commented Oct 8, 2017

View reviewed changes

nikomatsakis requested changes Oct 9, 2017

View reviewed changes

carols10cents added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Oct 9, 2017

Nashenas88 added 2 commits October 11, 2017 20:01

Move const qualifier from brackets to constant values and remove comm…

9423bee

…a from after identifier

Split lines longer than 100 columns

97fe353

kennytm added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 13, 2017

nikomatsakis reviewed Oct 13, 2017

View reviewed changes

kennytm added a commit to kennytm/rust that referenced this pull request Oct 13, 2017

Rollup merge of rust-lang#45110 - Nashenas88:master, r=arielb1

82f2c28

Improve newtype_index macro to handle description and constants consistently

kennytm mentioned this pull request Oct 13, 2017

Rollup of 14 pull requests #45261

Merged

bors merged commit 97fe353 into rust-lang:master Oct 13, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve newtype_index macro to handle description and constants consistently #45110

Improve newtype_index macro to handle description and constants consistently #45110

Nashenas88 commented Oct 8, 2017

rust-highfive commented Oct 8, 2017

Nashenas88 commented Oct 8, 2017

Nashenas88 Oct 8, 2017

nikomatsakis Oct 9, 2017

Nashenas88 Oct 8, 2017

spastorino commented Oct 8, 2017

nikomatsakis left a comment

nikomatsakis Oct 9, 2017

spastorino Oct 9, 2017 •

edited

Loading

Nashenas88 Oct 10, 2017

Nashenas88 Oct 10, 2017

nikomatsakis Oct 10, 2017

Nashenas88 Oct 12, 2017

nikomatsakis Oct 9, 2017

nikomatsakis Oct 9, 2017

arielb1 commented Oct 12, 2017

bors commented Oct 12, 2017

Mark-Simulacrum commented Oct 13, 2017

nikomatsakis Oct 13, 2017

Improve newtype_index macro to handle description and constants consistently #45110

Improve newtype_index macro to handle description and constants consistently #45110

Conversation

Nashenas88 commented Oct 8, 2017

rust-highfive commented Oct 8, 2017

Nashenas88 commented Oct 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spastorino commented Oct 8, 2017

nikomatsakis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spastorino Oct 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arielb1 commented Oct 12, 2017

bors commented Oct 12, 2017

Mark-Simulacrum commented Oct 13, 2017

Choose a reason for hiding this comment

spastorino Oct 9, 2017 •

edited

Loading