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

Implement `?` macro repetition #47752

Merged
merged 18 commits into from Feb 11, 2018

Conversation

Projects
None yet
7 participants
@mark-i-m
Copy link
Contributor

commented Jan 25, 2018

See rust-lang/rfcs#2298 (with disposition merge)

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

commented Jan 25, 2018

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @nikomatsakis (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.

@kennytm

This comment has been minimized.

Copy link
Member

commented Jan 25, 2018

Marking as blocked by #47732, #47603, and of course rust-lang/rfcs#2298. I don't think this can be easily reviewed with all those formatting change 😂

@mark-i-m

This comment has been minimized.

Copy link
Contributor Author

commented Jan 25, 2018

@kennytm I 100% agree. Thanks :)

@kennytm

This comment has been minimized.

Copy link
Member

commented Jan 26, 2018

@mark-i-m Please put the new ? syntax behind a feature gate since this is an unstable feature. You may refer to :vis (search for macro_vis_matcher) to know how this works.

@bors

This comment has been minimized.

Copy link
Contributor

commented Jan 26, 2018

☔️ The latest upstream changes (presumably #47748) made this pull request unmergeable. Please resolve the merge conflicts.

@mark-i-m mark-i-m force-pushed the mark-i-m:at-most-once-rep branch from 4652013 to abfdbdf Jan 26, 2018

@mark-i-m

This comment has been minimized.

Copy link
Contributor Author

commented Jan 26, 2018

@kennytm Done.

@mark-i-m

This comment has been minimized.

Copy link
Contributor Author

commented Jan 26, 2018

This doesn't have a tracking issue yet, so when that happens, I should update the feature gate...

@mark-i-m

This comment has been minimized.

Copy link
Contributor Author

commented Jan 26, 2018

Also the unstable book...

@nikomatsakis
Copy link
Contributor

left a comment

Good start, nice work! Left various comments, will re-review once addressed.

// #1 is `?` token, but it could be a Kleene::ZeroOrOne without a separator or it could
// be a `?` separator followed by any Kleene operator. We need to look ahead 1 token to
// find out which.
Ok(Ok(op)) => {

This comment has been minimized.

Copy link
@nikomatsakis

nikomatsakis Jan 29, 2018

Contributor

Nit: can we add assert_eq!(op, KleeneOp::ZeroOrOne); here? I find it helps catch mistakes when later somebody messes with the arms accidentally....

// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
// option. This file may not be copied, modified, or distributed
// except according to those terms.

This comment has been minimized.

Copy link
@nikomatsakis

nikomatsakis Jan 29, 2018

Contributor

I always appreciate a comment here explaining what this test is trying to do. Example:


Test that in $(a)?+, the ? is interpreted as a separator, but not for $(a)?. This test focuses on error cases.


macro_rules! bar {
($(a)?+) => {}
}

This comment has been minimized.

Copy link
@nikomatsakis

nikomatsakis Jan 29, 2018

Contributor

what about $(a)?*? we should test that too, no?


macro_rules! bar {
($(a)?+) => {}
}

This comment has been minimized.

Copy link
@nikomatsakis

nikomatsakis Jan 29, 2018

Contributor

what about $(a)?*? we should test that too, no?

// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
// option. This file may not be copied, modified, or distributed
// except according to those terms.

This comment has been minimized.

Copy link
@nikomatsakis

nikomatsakis Jan 29, 2018

Contributor

comment =)

// except according to those terms.

macro_rules! foo {
($(a)?) => {}

This comment has been minimized.

Copy link
@nikomatsakis

nikomatsakis Jan 29, 2018

Contributor

these tests would be better if they had things in the body that made use of a

@@ -11,7 +11,7 @@
#![allow(unused_macros)]

macro_rules! assign {
(($($a:tt)*) = ($($b:tt))*) => { //~ ERROR expected `*` or `+`
(($($a:tt)*) = ($($b:tt))*) => { //~ ERROR 14:22: 14:29: expected one of: `*`, `+`, or `?`

This comment has been minimized.

Copy link
@nikomatsakis

nikomatsakis Jan 29, 2018

Contributor

Please move this to a ui test instead of hard-coding spans and things (i.e., the 14:22)

This comment has been minimized.

Copy link
@mark-i-m

mark-i-m Jan 29, 2018

Author Contributor

Oh, I intended to remove the line/col numbers but forgot... Should I still move to a ui test if I remove them?

This comment has been minimized.

Copy link
@nikomatsakis

nikomatsakis Jan 30, 2018

Contributor

Doesn't matter so much. I believe ui tests are generally "the future" but this test can stay a compile-fail test.

@@ -12,13 +12,19 @@ macro_rules! foo {
($(a)?) => {}
}

macro_rules! baz {
($(a),?) => {} // comma separator is meaningless for `?`

This comment has been minimized.

Copy link
@nikomatsakis

nikomatsakis Jan 29, 2018

Contributor

wait-- so $(a),? is accepted but the , is just a no-op? seems like it should be an error

// option. This file may not be copied, modified, or distributed
// except according to those terms.

// Test that the MSP430 interrupt ABI cannot be used when msp430_interrupt

This comment has been minimized.

Copy link
@nikomatsakis

nikomatsakis Jan 29, 2018

Contributor

This comment is wrong =)

@mark-i-m

This comment has been minimized.

Copy link
Contributor Author

commented Jan 29, 2018

@nikomatsakis

wait-- so $(a),? is accepted but the , is just a no-op? seems like it should be an error

How would you feel about a lint instead? I would like to keep the separator simply for consistency with + and * even though having a separator on "at most once" is silly.

@mark-i-m

This comment has been minimized.

Copy link
Contributor Author

commented Jan 29, 2018

@nikomatsakis I think I addressed your comments. You may want to wait on a re-review until #47732, #47603 are merged, as they will make the diffs easier to read...

@mark-i-m mark-i-m changed the title [WIP] Implement `?` macro repetition Implement `?` macro repetition Jan 29, 2018

@bors

This comment has been minimized.

Copy link
Contributor

commented Jan 30, 2018

☔️ The latest upstream changes (presumably #47870) made this pull request unmergeable. Please resolve the merge conflicts.

@mark-i-m

This comment has been minimized.

Copy link
Contributor Author

commented Jan 30, 2018

until #47732, #47603 are merged

They have merged now 🎉 ... I will rebase shortly

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

commented Jan 30, 2018

How would you feel about a lint instead? I would like to keep the separator simply for consistency with + and * even though having a separator on "at most once" is silly.

Maybe we should bring this up in the RFC thread and get others' opinions. I don't have a very strong opinion, but it seems like an error would be better to me.

@mark-i-m

This comment has been minimized.

Copy link
Contributor Author

commented Feb 9, 2018

Updated

@CAD97

This comment has been minimized.

Copy link
Contributor

commented Feb 9, 2018

Failure is due to added (see tracking issue):

[00:53:32] ---- [ui] ui/feature-gate-macro_at_most_once_rep.rs stdout ----
[00:53:32] 	diff of stderr:
[00:53:32] 
[00:53:32] -	error[E0658]: Using the `?` macro Kleene operator for "at most one" repetition is unstable
[00:53:32] +	error[E0658]: Using the `?` macro Kleene operator for "at most one" repetition is unstable (see issue #48075)
@mark-i-m

This comment has been minimized.

Copy link
Contributor Author

commented Feb 9, 2018

Thanks! Fixed.

@mark-i-m

This comment has been minimized.

Copy link
Contributor Author

commented Feb 9, 2018

hmm... tidy is now complaining that that line is too long 🤕

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

commented Feb 9, 2018

@bors r+

@bors

This comment has been minimized.

Copy link
Contributor

commented Feb 9, 2018

📌 Commit b92e542 has been approved by nikomatsakis

@bors

This comment has been minimized.

Copy link
Contributor

commented Feb 11, 2018

⌛️ Testing commit b92e542 with merge e8b05f5...

bors added a commit that referenced this pull request Feb 11, 2018

Auto merge of #47752 - mark-i-m:at-most-once-rep, r=nikomatsakis
Implement `?` macro repetition

See rust-lang/rfcs#2298 (with disposition merge)
@bors

This comment has been minimized.

Copy link
Contributor

commented Feb 11, 2018

💔 Test failed - status-appveyor

@kennytm

This comment has been minimized.

Copy link
Member

commented Feb 11, 2018

@bors retry #48116

@bors

This comment has been minimized.

Copy link
Contributor

commented Feb 11, 2018

⌛️ Testing commit b92e542 with merge b8398d9...

bors added a commit that referenced this pull request Feb 11, 2018

Auto merge of #47752 - mark-i-m:at-most-once-rep, r=nikomatsakis
Implement `?` macro repetition

See rust-lang/rfcs#2298 (with disposition merge)
@bors

This comment has been minimized.

Copy link
Contributor

commented Feb 11, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: nikomatsakis
Pushing b8398d9 to master...

@bors bors merged commit b92e542 into rust-lang:master Feb 11, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details

@mark-i-m mark-i-m deleted the mark-i-m:at-most-once-rep branch Nov 14, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.