[PATCH] D33339: Fix valid-for-expr ellipses eaten as invalid decl

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added a comment.

In https://reviews.llvm.org/D33339#759125, @rsmith wrote:

> Should I assume our "misplaced ellipsis" diagnostic requires that we disambiguate the ill-formed ellipsis-after-declarator-id as a declarator in some cases? If so, do we have tests for that somewhere?


Tests for the diagnostics are in `clang/test/FixIt/fixit-cxx0x.cpp` and `clang/test/Parser/cxx11-templates.cpp`.
The `check-all` target passes even if the ellipsis-after-declarator-id disambiguation as a declarator is removed entirely. The disambiguation process is not needed involved in many cases, and when it is, can choose the declarative interpretation for other reasons.

I find the most interesting cases affected by the patch here to be those involving disambiguation between the declaration of a function template and that of a variable template.
The misplaced-ellipsis diagnostic will still trigger (if we accept non-trailing stray ellipses) on a case like

  template <typename ...T> int f(T (t) ...(int), int (x));

which, although it works, it not very motivating.

Removing the `(int)` turns it into a possibly-valid variable template declaration.
Removing the parentheses around `t` (whether or not the `(int)` is there) makes the parser decide to parse as a function declaration (and the misplaced-ellipsis diagnostic is triggered) regardless of the treatment of "stray" ellipses.
The logic implemented here does not generate the misplaced-ellipsis diagnostic for

  template <typename ...T> int f(T (t) ..., int x);

So, on the whole, the stray ellipsis treatment is both too complicated and not complicated enough.



================
Comment at: include/clang/Parse/Parser.h:2138
+  TPResult TryParseDeclarator(bool mayBeAbstract, bool mayHaveIdentifier = true,
+                              bool mayHaveTrailingStrayEllipsis = true);
   TPResult
----------------
rsmith wrote:
> Given that this flag is enabling parsing of things that are *not* valid declarators, I think the default should be the other way around. If some calling code believes it's safe to parse a trailing ellipsis as part of a declarator, it should be explicitly opting into that.
Okay; will do.


================
Comment at: lib/Parse/ParseTentative.cpp:944
+        (mayHaveTrailingStrayEllipsis ||
+         !(NextToken().is(tok::r_paren) || NextToken().is(tok::comma))))
       ConsumeToken();
----------------
rsmith wrote:
> hubert.reinterpretcast wrote:
> > The check for what qualifies as "trailing" is not the strongest, but I find it to be the most straightforward change that should do the job. One alternative is to track whether an ellipsis was consumed within the current loop iteration.
> Use `!NextToken().isOneOf(tok::r_paren, tok::comma)` here.
Will do.


https://reviews.llvm.org/D33339