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

Argiris Kirtzidis akyrtzi at gmail.com
Sun Aug 24 08:14:12 PDT 2008

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

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.


> +  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() {
  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?


> +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!

-------------- 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>

More information about the cfe-dev mailing list