p4lang / p4-spec

Apache License 2.0
178 stars 80 forks source link

Update spec grammar to more closely match open source p4c implementation #1258

Closed jafingerhut closed 1 year ago

jafingerhut commented 1 year ago

In particular, to match the changes made to the p4c implementation with this commit: https://github.com/p4lang/p4c/commit/64683c958b64b15a5a20c04c5c793a25b6fdb748

jafingerhut commented 1 year ago

@jonathan-dilorenzo I do not know why, but when I click on the reviewers to try to add jonathan-dilorenzo to the list of reviewers, it does not let me add that user name, so pinging you in a comment instead for your review, if you have time.

jnfoster commented 1 year ago

This looks good to me. Let me review it carefully though.

jafingerhut commented 1 year ago

A couple of scripts in the p4-spec repo described in this README.md file can help do a diff between the p4c grammar and the spec grammar file: https://github.com/p4lang/p4-spec/blob/main/p4-16/spec/scripts/README.md

Those are the steps I use to find such differences and make the grammars closer to each other. They are not identical, but they are fairly close to each other.

jafingerhut commented 1 year ago

Looks like it matches the p4c grammar to me. I don't see anywhere we'd actually want to allow PRIORITY as a nonTypeName, but maybe it exists?

For the nonterminals like nonTypeName and what they can expand to, I honestly do not understand the issues involved. @mihaibudiu probably does, as I am simply copying what he did from the p4c implementation when he made some changes. My understanding is that somehow those most recent changes enabled priority to be used as a P4-developer-chosen identifier name in at least some contexts where the previous grammar and p4c implementation prevented that.

Correction: I believe @ChrisDodd was the one who made those p4c changes, not Mihai. Apologies for the misattribution.

jonathan-dilorenzo commented 1 year ago

Ah, that would make sense. And I suppose one might want that.

jonathan-dilorenzo commented 1 year ago

Alright. Seeing consensus I will merge this.