:vis matcher for macro_rules#41012

durka · 2017-04-02T05:57:40Z

Resurrection of @DanielKeep's implementation posted with RFC 1575.

@jseyfried was of the opinion that this doesn't need an RFC.

Needed before merge:

sign-off from @DanielKeep since I stole his code
feature gate
docs

rust-highfive · 2017-04-02T05:57:50Z

r? @pnkfelix

(rust_highfive has picked a reviewer for you, use r? to override)

DanielKeep · 2017-04-02T07:01:18Z

sign-off from @DanielKeep since I graciously resurrected his code lazy hack that he is

jseyfried

Code LGTM

jseyfried · 2017-04-02T07:11:33Z

src/libsyntax/ext/tt/macro_rules.rs

Why do we just allow Comma and ModSep? Seems arbitrary to me.

If I remember correctly, I allowed ModSep because you can already have pub ::path::to::a_type that you might want to parse and special-case. I allowed comma because having some kind of sequence-delimiting token is useful. If you're passing bits of parsed input around, you might have inner!($thing, $a_vis, $more_tts). Everything else was excluded based on wanting to err on the side of being conservative.

I allowed ModSep because you can already have pub ::path::to::a_type

This seems quite inconsistent, you can already have pub ARBITRARY_TYPE, but only a subset of paths is allowed.

This should probably use all can_begin_type tokens (in addition to comma) + ty/path/ident for MetaVarDecls.
Or all is_path_start tokens (in addition to comma) + ty/path/ident for MetaVarDecls if we want to be conservative.

can_begin_type seems good, tuple declarations really restrict us from changing visibility syntax too much in the future (as we saw during the pub(restricted) debate).

petrochenkov · 2017-04-02T09:14:00Z

src/libsyntax/parse/token.rs

Now the comment // These are not exposed to macros .... applies to NtVis, it's better moved higher in the list.

durka · 2017-04-02T19:05:49Z

src/test/run-pass/macro-pub-matcher.rs

@DanielKeep why did you not use :vis at the top level of this macro??

durka · 2017-04-03T00:12:01Z

Comments addressed.

petrochenkov · 2017-04-03T08:21:21Z

LGTM, but needs @rust-lang/lang decision

joshtriplett · 2017-04-03T15:47:35Z

As a note: I observed someone needing this on the #rust IRC channel a few days ago, to avoid hard-coding "pub".

nrc · 2017-04-03T23:56:05Z

@rfcbot fcp merge

rfcbot · 2017-04-03T23:56:06Z

Team member @nrc has proposed to merge this. The next step is review by the rest of the tagged teams:

No concerns currently listed.

Once these reviewers reach consensus, this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

withoutboats · 2017-04-04T00:21:05Z

@rfcbot reviewed

pnkfelix · 2017-04-04T12:53:06Z

src/doc/unstable-book/src/macro-vis-matcher.md

Hmm, I'm feeling dense: Why can a vis variable be followed by a comma?

To quote @DanielKeep's rationale from above (hidden comment):

I allowed comma because having some kind of sequence-delimiting token is useful. If you're passing bits of parsed input around, you might have inner!($thing, $a_vis, $more_tts).

This isn't required, we can exclude it and you'd have to use ($a_vis) to pass around parsed visibility fragments within macros.

It seems harmless to me. I can't imagine using , as an "operator" in a visibility specifier (at last outside of parens or something).

@pnkfelix are you still worried about this? Should we exclude it in order to get this PR into FCP? I just don't want this to continue languishing for too long...

no its okay; thanks for your patience.

bors · 2017-04-07T07:23:49Z

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

pnkfelix · 2017-04-14T15:09:37Z

@rfcbot reviewed

rfcbot · 2017-04-14T15:56:23Z

🔔 This is now entering its final comment period, as per the review above. 🔔

durka · 2017-04-15T03:48:05Z

Rebased.

bors · 2017-04-15T04:48:23Z

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

durka · 2017-04-15T19:07:20Z

Rebased.

aturon · 2017-04-17T15:57:40Z

@bors r=petrochenkov

bors · 2017-04-17T15:57:40Z

📌 Commit cfa51f2 has been approved by petrochenkov

@DanielKeep

:vis matcher for macro_rules Resurrection of @DanielKeep's implementation posted with [RFC 1575](rust-lang/rfcs#1575). @jseyfried was of the opinion that this doesn't need an RFC. Needed before merge: - [x] sign-off from @DanielKeep since I stole his code - [x] feature gate - [x] docs

Rollup of 3 pull requests - Successful merges: #41012, #41280, #41290 - Failed merges:

rust-highfive assigned pnkfelix Apr 2, 2017

jseyfried reviewed Apr 2, 2017

View reviewed changes

petrochenkov reviewed Apr 2, 2017

View reviewed changes

durka commented Apr 2, 2017

View reviewed changes

src/test/run-pass/macro-pub-matcher.rs Outdated

Copy link

Contributor Author

durka Apr 2, 2017

Choose a reason for hiding this comment

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

@DanielKeep why did you not use :vis at the top level of this macro??

durka mentioned this pull request Apr 2, 2017

Tracking issue for :vis macro matcher #41022

Closed

durka force-pushed the vis-matcher branch from cf757e1 to bf277fa Compare April 3, 2017 00:11

durka changed the title ~~[WIP] :vis matcher for macro_rules~~ :vis matcher for macro_rules Apr 3, 2017

durka force-pushed the vis-matcher branch 2 times, most recently from 6f03265 to 41dcfec Compare April 3, 2017 02:10

petrochenkov added I-nominated T-lang Relevant to the language team labels Apr 3, 2017

pnkfelix reviewed Apr 4, 2017

View reviewed changes

nrc removed the I-nominated label Apr 6, 2017

alexcrichton mentioned this pull request Apr 12, 2017

support pub(restricted) in thread_local! #40984

Closed

Mark-Simulacrum added the S-waiting-on-team label Apr 14, 2017

rfcbot added the final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. label Apr 14, 2017

Mark-Simulacrum added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-team labels Apr 14, 2017

durka force-pushed the vis-matcher branch from 41dcfec to e2d9409 Compare April 15, 2017 03:47

DanielKeep and others added 9 commits April 15, 2017 19:06

Implementation of the vis macro matcher.

a248949

update :vis implementation to current rust

d53e413

update print_visibility for new pub(restricted) syntax

06411c4

parse interpolated visibility tokens

16010c2

move NtVis enum variant to stave off comment rot

06d3233

widen :vis follow set

37459e1

document :vis in unstable book

1d46805

feature gate :vis matcher

e0cd766

reduce macro rule duplication in test

51c8af5

durka force-pushed the vis-matcher branch from e2d9409 to 51c8af5 Compare April 15, 2017 19:07

satisfy completely useless tidy check

cfa51f2

aturon 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 Apr 17, 2017

frewsxcv mentioned this pull request Apr 17, 2017

Rollup of 3 pull requests #41345

Merged

bors added a commit that referenced this pull request Apr 17, 2017

Auto merge of #41345 - frewsxcv:rollup, r=frewsxcv

5516bcc

Rollup of 3 pull requests - Successful merges: #41012, #41280, #41290 - Failed merges:

bors merged commit cfa51f2 into rust-lang:master Apr 17, 2017

petrochenkov mentioned this pull request May 4, 2017

Tracking issue for adding a lifetime specifier to macro_rules! #34303

Closed

Geal mentioned this pull request Oct 6, 2018

simplify named! macros with the :vis macro arg type rust-bakery/nom#838

Closed

Uh oh!

Conversation

durka commented Apr 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rust-highfive commented Apr 2, 2017

Uh oh!

DanielKeep commented Apr 2, 2017

Uh oh!

jseyfried left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

durka commented Apr 3, 2017

Uh oh!

petrochenkov commented Apr 3, 2017

Uh oh!

joshtriplett commented Apr 3, 2017

Uh oh!

nrc commented Apr 3, 2017

Uh oh!

rfcbot commented Apr 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

withoutboats commented Apr 4, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

durka Apr 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bors commented Apr 7, 2017

Uh oh!

pnkfelix commented Apr 14, 2017

Uh oh!

rfcbot commented Apr 14, 2017

Uh oh!

durka commented Apr 15, 2017

Uh oh!

bors commented Apr 15, 2017

Uh oh!

durka commented Apr 15, 2017

Uh oh!

aturon commented Apr 17, 2017

Uh oh!

bors commented Apr 17, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

14 participants

durka commented Apr 2, 2017 •

edited

Loading

rfcbot commented Apr 3, 2017 •

edited

Loading

durka Apr 4, 2017 •

edited

Loading