[PATCH] D130337: [pseudo] Eliminate multiple-specified-types ambiguities using guards

[Sam McCall via Phabricator via cfe-commits]

sammccall marked 2 inline comments as done.
sammccall added inline comments.


================
Comment at: clang-tools-extra/pseudo/lib/cxx/cxx.bnf:353
 decl-specifier-seq := decl-specifier
-decl-specifier-seq := decl-specifier decl-specifier-seq
+decl-specifier-seq := decl-specifier decl-specifier-seq [guard]
 storage-class-specifier := STATIC
----------------
hokein wrote:
> offtopic comment: The sad bit of the `RuleID` approach (vs `guard=SingleType`) is that we don't really know what kind of the guard is by reading the grammar file only.
> 
> I think this is critical information, and worth to keep in the grammar file. (ideas: add comments, or bring the `guard=SingleType` in the grammar again, but we ignore the `guard` value in the grammar parsing).
Yeah, the current balance doesn't feel obviously right.

I'd like to leave this for the time being, because of the various options (remove the annotations, replace them with comments, bring back values), i'm not sure there's a clear winner.

I have a suspicion that while it's appealing now to at least reference here all the restrictions that may apply, when we add "soft" disambiguation based on scoring it may not be so appealing as we won't be documenting the things that affect the parse in practice.


================
Comment at: clang-tools-extra/pseudo/lib/cxx/cxx.bnf:385
+builtin-type := BOOL
 simple-type-specifier := SHORT
+builtin-type := INT
----------------
hokein wrote:
> I think the reason to leave SHORT/LONG/SIGNED UNSIGNED as-is is that they can combined with other type (e.g. short int).
> 
> Can we group them together, and add a comment?
Grouped them.
I don't think the idea that SHORT is a specifier but not actually a type needs to be spelled out, but added a comment about `builtin-type` (which is nonstandard) which hints at this.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D130337/new/

https://reviews.llvm.org/D130337