[cfe-dev] [PATCH] C++ decl/expr ambiguity resolution

[Argiris Kirtzidis]

New patch attached, comments follow below:

Eli Friedman wrote:
> I agree that part of the standard isn't very well written.  That said,
> for the cases that are supposed to parse as expressions, the ambiguity
> resolution doesn't actually have any effect; the grammar isn't
> ambiguous for those cases in the first place.  The grammar is just
> very annoying for the parser. :)
>
> Comeau seems to agree with my interpretation; it accepts all the
> examples that you've given.
>   

Chris Lattner wrote:
>
> I agree with Eli.  The standard is very clear here, and it sounds like 
> these are bugs in GCC.  Specifically, if a whole statement can be 
> parsed as a declaration, it should be.  If the whole statement can't, 
> and it is legal as an expression, then it must be parsed as a 
> statement.  Rejecting a legal expression because it doesn't fit the 
> declaration syntax would be a bug.

Ok, this sounds reasonable. I was leaning on the more optimistic side 
that these are not GCC bugs but "design choices" :-)
In the new patch, disambiguation follows the Comeau way and all the 
declarators are checked. Note that "int(x) = {0}, ++x;" will be 
interpreted as ill-formed expression.

>
> Another related but purely orthogonal issue is the efficiency 
> concern.  It is preferable to avoid backtracking in obvious cases 
> where we know that something is a statement.  For example, it is 
> impossible for a declaration to start with the 'do' keyword, so if we 
> see that, we can avoid the cost of starting/reverting backtracking.

It's not that costly to start/revert (without consuming tokens 
in-between) but we can certainly avoid it, the new patch does.


Eli Friedman wrote:
> In any case, it's probably a good idea to emit a warning for code that
> ends up falling into ambiguity resolution, i.e. anything that gets
> past the call to TryParseDeclarationSpecifier from
> TryParseSimpleDeclaration in your patch.  It's very likely that such
> code doesn't do what the author intends, and it's usually very easy to
> make the intention explicit (either by removing parentheses around the
> variable declaration, or putting parentheses around the function-style
> cast).
>   

The warning seems like a good idea, I've added one and it works like this:

t.cpp:7:3: warning: statement was disambiguated as declaration
  int(x);
  ^~~~~~~

Just to reduce these warnings a bit, I don't emit it when we 
disambiguated a declaration starting with 'void', which almost certainly 
is an obvious function declaration.

> Comments on the patch follow:
>
> +      while (Tok.is(tok::kw_const) || Tok.is(tok::kw_volatile))
> +        ConsumeToken();
>
> Doesn't the presence of const or volatile prove that this is a
> declaration?  I don't think this actually affects the semantics, but
> it seems cleaner to exit once we can prove something.
>   

Could be that they are misplaced in an expression statement. GCC, MSVC, 
and Comeau agree:

void f() {
  int x;
  int (* const x)++;
}

"ComeauTest.c", line 3: error: expected an expression
    int (* const x), ++x;
           ^


> Also, shouldn't we be handling restrict here as well?  It's not
> standard C++, but I think people use it.
>   

Done.

> +  if (Tok.is(tok::identifier)) {
> +    // declarator-id
> +    ConsumeToken();
>
> Don't you need some sort of check to see whether this is a type?
>   

It doesn't matter whether it is a type or not. If it is a valid 
declaration, it shadows the type-name. If the type is in the same scope 
it may erroneously redeclare it but disambiguation doesn't take scope 
information into account.

> Consider something like the following:
> class T {public: T(int a) {} T() {} int operator*() {return 0;}};
> void a() {T(*T());} // Must be expression
>   

This is actually a function declaration, the same as "T* T();". GCC agrees:

class T {public: T(int a) {} T() {} int operator*() {return 0;}};
void a() {
  T(*T());
  int T;
}

t.cpp: In function 'void a()':
t.cpp:4: error: 'int T' redeclared as different kind of symbol
t.cpp:3: error: previous declaration of 'T* T()'


So is Comeau:

"ComeauTest.c", line 4: error: "T" has already been declared in the current scope
    int T;
        ^

<sarcasm> I love that we are debating whether something is a declaration 
or an expression, the C++ grammar is so beautiful! </sarcasm>

> +  case tok::kw_const:
> +  case tok::kw_volatile:
>
> Again, handle restrict?
>   

Done.

> +Parser::TentativeParsingResult
> +Parser::TryParseParameterDeclarationClause(SourceLocation LParenLoc) {
> +  TentativeParsingResult TPR = TryParseDeclarationSpecifier();
> +
> +  if (TPR != TPR_false) {
>
> What the heck is this if statement supposed to be testing for?  Cases
> like empty parameter declarations?  This whole area looks rather
> strange.  I haven't worked out whether this has any visible effects,
> but it seems like it would be a lot clearer to explicitly check for
> the cases where TryParseDeclarationSpecifier returns false, but there
> is in fact a valid parameter list (the only cases I can think of are
> an empty parameter list and a list with only "...").
>   

I've improved the readability of this function.

> Otherwise, I think it looks fine; that said, someone who knows the
> parser better should review it as well.
>   

Thank you for your feedback!


-Argiris
-------------- next part --------------
A non-text attachment was scrubbed...
Name: tentative-parse-2.patch
Type: text/x-diff
Size: 30621 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20080824/acf8d877/attachment.patch>