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 register_diagnostics macro #27067

Closed
wants to merge 2 commits into from

Conversation

GuillaumeGomez
Copy link
Member

Now the macro argument list can be finished by a comma (not sure this is correct english...).

cc @tamird
r? @bluss

@tamird
Copy link
Contributor

tamird commented Jul 16, 2015

you can also make the comma required by changing the existing thing to ($($code:tt,)*) => (; I'd personally prefer that. Also true of the other nearby macros (i think there are 3 or 4)

@GuillaumeGomez
Copy link
Member Author

I can add this "feature" for others too, of course. However, I like to have choice to finish the macro with or without a comma. Let's see others' opinions.

@emberian
Copy link
Member

This looks fine to me. The Rust Way is to always allow trailing comma but never require it, so this seems consistent with that.

@emberian
Copy link
Member

@bors: r+ rollup

@bors
Copy link
Collaborator

bors commented Jul 17, 2015

📌 Commit a5d8c43 has been approved by cmr

@emberian
Copy link
Member

@bors: r- (pending more changes)

@emberian
Copy link
Member

@bors: r+ rollup

@bors
Copy link
Collaborator

bors commented Jul 17, 2015

📌 Commit 6e58043 has been approved by cmr

@bors
Copy link
Collaborator

bors commented Jul 17, 2015

⌛ Testing commit 6e58043 with merge b429d3d...

@bors
Copy link
Collaborator

bors commented Jul 17, 2015

💔 Test failed - auto-mac-64-nopt-t

@alexcrichton
Copy link
Member

@bors: retry

On Fri, Jul 17, 2015 at 7:27 AM, bors notifications@github.com wrote:

[image: 💔] Test failed - auto-mac-64-nopt-t
http://buildbot.rust-lang.org/builders/auto-mac-64-nopt-t/builds/5754


Reply to this email directly or view it on GitHub
#27067 (comment).

@bors
Copy link
Collaborator

bors commented Jul 17, 2015

⌛ Testing commit 6e58043 with merge 0062146...

@bors
Copy link
Collaborator

bors commented Jul 17, 2015

💔 Test failed - auto-linux-64-nopt-t

@alexcrichton
Copy link
Member

@bors: retry

On Fri, Jul 17, 2015 at 8:31 AM, bors notifications@github.com wrote:

[image: 💔] Test failed - auto-linux-64-nopt-t
http://buildbot.rust-lang.org/builders/auto-linux-64-nopt-t/builds/5684


Reply to this email directly or view it on GitHub
#27067 (comment).

Manishearth added a commit to Manishearth/rust that referenced this pull request Jul 17, 2015
Now the macro argument list can be finished by a comma (not sure this is correct english...).

cc @tamird
r? @bluss
bors added a commit that referenced this pull request Jul 17, 2015
@GuillaumeGomez
Copy link
Member Author

Merged in #27098. I close.

@GuillaumeGomez GuillaumeGomez deleted the patch-1 branch July 20, 2015 14:50
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.

6 participants