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

Amend macro future proofing RFC #733

Closed
wants to merge 4 commits into from

Conversation

vadimcn
Copy link
Contributor

@vadimcn vadimcn commented Jan 24, 2015

Proposed changes:

  • Add Semicolon to FOLLOW(ty).
  • Modify algorithm to allow repetitions after NTs.

(rendered)

@nikomatsakis
Copy link
Contributor

It seems like the RFC is incomplete: FIRST needs to be spelled out.

@vadimcn
Copy link
Contributor Author

vadimcn commented Jan 30, 2015

@cmr, I would like to clarify something:
The RFC does not mention handling of TtDelimited token trees. Are they supposed to be implicitly flattened into the sequence of constituent tokens? If so, shouldn't the last line of this section say "(Note that open and close delimiters are valid following any NT.)" ?

@vadimcn vadimcn force-pushed the amend-macro-future-proofing branch 2 times, most recently from 852f861 to ad390bd Compare January 30, 2015 06:34
@vadimcn vadimcn force-pushed the amend-macro-future-proofing branch from ad390bd to 38ff99a Compare January 30, 2015 07:14
@vadimcn
Copy link
Contributor Author

vadimcn commented Jan 30, 2015

Okay, updated my PR.

@nikomatsakis
Copy link
Contributor

@vadimcn thanks. I'll try to read it and give it some thought. In the meantime, though, @pnkfelix is going to shepherd this PR.

@emberian
Copy link
Member

@vadimcn Yes, your understanding is correct.

@mzabaluev
Copy link
Contributor

Could this help solve #773?

4. Find `S`, the set of possible successors of `t`:
1. If `TAIL(M)` is empty, set `S = F`,
2. Else, `S = LEADERS(TAIL(M))`.
5. If `t` is a simple NT, check that `S` is a subset of `FOLLOW(t)`.
Copy link
Member

Choose a reason for hiding this comment

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

what is the distinction between simple and complex non-terminal? I do not see that in the "Terminology" section.

Update: Sorry, I see the definition for "simple", up above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just used the original @cmr's definitions of 'simple' and complex'. I'll add a clarification as you asked above.

@emberian
Copy link
Member

Looks like some rebasings have disappeared some of the comments I previously made. Will re-review, but @pnkfelix's and @nikomatsakis's comments are on-point.

@kmcallister
Copy link
Contributor

Did not read yet, but +1 for allowing repetition after a metavariable. The inability to do

$x:tt $($xs:tt)*

is a serious limitation with fancy recursive macros. And it should be completely un-problematic in the case of tt.

Renamed `LEADERS` to `FIRST`
@vadimcn
Copy link
Contributor Author

vadimcn commented Feb 26, 2015

@cmr, @nikomatsakis: ping

@aturon
Copy link
Member

aturon commented Mar 5, 2015

ping @pnkfelix

@kennytm
Copy link
Member

kennytm commented Mar 5, 2015

While this is still open, could we also add | and if to FOLLOW(pat)? (cc rust-lang/rust#20843)

@pnkfelix
Copy link
Member

Okay, so I finally got a chance to go through this in some detail.

(I had not really read this RFC carefully enough in the past, so I got caught up in relatively minor issues in its presentation that are not an issue with this PR in particular; hopefully I will have a chance in the near future, while this is fresh in my mind, to try to put up a PR with some revisions to the text to help clarify it.)

  • I think it would be good to add some examples of the CHECK algorithm's behavior on various inputs. I know the original RFC did not have this, but I think it should have, and there's no reason we cannot correct that now. I will help with this (i.e. I'll put up some text somewhere for @vadimcn to incorporate.)
  • I think there is a bug in this additions being made here, in a corner-case for the FIRST function. The standard definition for FIRST returns a set made up of terminals plus (potentially) a special "epsilon" marker signifying that the input sequence may rewrite to the empty string. That can arise for us here (despite effort in this definition to try to avoid it), and needs to be handled. I'm about to go have dinner so I cannot put in a formal example, but the kind of scenario I am worried about is something like $($($(i:expr)),* $($(j:expr)),*) something. I will write up more later.

@Stebalien
Copy link
Contributor

While this is still open, could we also add | and if to FOLLOW(pat)? (cc rust-lang/rust#20843)

Also in for parsing for loop syntax (for $p:pat in iter).

@nrc nrc added the T-lang Relevant to the language team, which will review and decide on the RFC. label May 28, 2015
@huonw
Copy link
Member

huonw commented Jul 31, 2015

cc #1209

@pnkfelix
Copy link
Member

@vadimcn have you given any thought to the feedback I wrote above?

(I know I promised to write up more later, so I cannot claim that I am not without blame w.r.t. the radio silence here.)

Anyway, let me know if you think you'll be able to address the issues above. If you won't have the time, I can try to take on this RFC myself (i.e. try to figure out what are the edits that I want, and put my my own PR with them...)

@vadimcn
Copy link
Contributor Author

vadimcn commented Aug 1, 2015

@pnkfelix: sorry this slipped through the cracks of my attention. I'll take another look tonight.

@vadimcn
Copy link
Contributor Author

vadimcn commented Aug 3, 2015

@pnkfelix: indeed, NT matching an empty string seems to be a problem. I've updated the PR to address this.

@nikomatsakis
Copy link
Contributor

What's the status here?

@pnkfelix
Copy link
Member

pnkfelix commented Jan 7, 2016

So I managed to spend ages working on the more invasive macro future-proofing amendment #1384 but I failed to incorporate the suggestions from this RFC in that effort. Sorry about that, I don't know how I overlooked it.

Update 1: Oh wait, no, I did put Semi into FOLLOW(ty) in #1384; I just forgot that I had done so. I'm going to check on how the amendment's handling of repetitions compares to the suggestion here.

@pnkfelix
Copy link
Member

Okay, #1384 handles repetitions in the manner that I think this PR was asking for:

macro_rules! foo { ($a:expr $( , $b:expr ),*) => {} }

compiles fine on Nightly today (but is rejected by Stable right now, of course).

So, given that we now handle sequence repetitions, and that Semi is in FOLLOW(ty), I think #1384 has subsumed this proposed amendment.

Thank you for your patience, @vadimcn

@pnkfelix pnkfelix closed this Jan 14, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-lang Relevant to the language team, which will review and decide on the RFC.
Projects
None yet
Development

Successfully merging this pull request may close these issues.