[Parse] Fix `TryParsePtrOperatorSeq`.

[michele.scandale]

Summary

The syntax rules for ptr-operator allow attributes after *, &,
&&, therefore we should be able to parse the following:

void fn() {
    void (*[[attr]] x)() = &fn;
    void (&[[attr]] y)() = fn;
    void (&&[[attr]] z)() = fn;
}

However the current logic in TryParsePtrOperatorSeq does not consider
the presence of attributes leading to unexpected parsing errors.

Moreover we should also consider _Atomic a possible qualifier that can
appear after the sequence of attribute specifiers.

[michele.scandale]

Missed GNU style attributes

[rjmccall]

clang/lib/Parse/ParseTentative.cpp

798

Do you want to check for double-brackets here?

[michele.scandale]

Check for double brackets for C++11 style attributes, and outline the loop to skip attributes in TrySkipAttributes to fix also the other use in TryConsumeDeclarationSpecifier

[michele.scandale]

clang/lib/Parse/ParseTentative.cpp

798

The copy of the code I was replicating here wasn't doing that. But it makes sense to check at least that.

[rjmccall]

clang/lib/Parse/ParseTentative.cpp

793

Okay, great. The check for adjacent squares at the end makes sure this doesn't catch an ObjC message send, in case this is ever used in a context where there's attribute/expression ambiguity; that might be worth a comment. Otherwise LGTM.

[michele.scandale]

Add comment about ObjC message send being rejected + rebase

[michele.scandale]

Assuming this is fine, I would need someone landing this change on my behalf since I do not have commit rights. Thanks!

clang/lib/Parse/ParseTentative.cpp

793

Comment added. Let me know if the wording is fine.

[rjmccall]

LGTM.

[michele.scandale]

@rsmith: is this fine for you as well?

If this is ready for landing, I would appreciate if somebody can land this on my behalf since I do not have commit rights. Thanks!

[rsmith]

clang/include/clang/Parse/Parser.h

2444 ↗

(On Diff #245073)

Stray "of' here. (And an 's' after "attribute-specifier" would improve readability.)

[michele.scandale]

Fixed comment wording + rebase.

[aaron.ballman]

clang/lib/Parse/ParseTentative.cpp

786

Do we need to also skip other keyword attributes as well as alignas? For instance, tok::kw__Noreturn or tok::kw__Alignas

792

I think this gets the parsing logic wrong when there is a balanced [] that is not part of the attribute introducer syntax, such as finding an array in the middle of the attribute argument clause. A test case would be handy, like:

constexpr int foo[] = { 2, 4 };
[[gnu::assume_aligned(foo[0])]] int *func() { return nullptr; }

You should probably use a BalancedDelimiterTracker to handle the brackets.

802

If we're skipping __attribute__(()), this looks to miss the second closing right paren. Also, this logic won't work if we need to skip _Noreturn, which has no parens.

This branch is missing test coverage.

[rsmith]

clang/lib/Parse/ParseTentative.cpp

786

_Alignas and _Noreturn are their own kinds of decl-specifiers, not attributes. In particular, they're not syntactically valid in any of the places that call this (after a * or a tag keyword). It looks like we're missing support for them, but we should recognize them in isCXXDeclarationSpecifier, not here -- but I think that's unrelated to this patch.

792

SkipUntil properly handles nested brackets; I think this is fine (though more testcases can't hurt!). I don't think BalancedDelimiterTracker would help -- this function can intentionally terminate with unbalanced delimiters, if it bails out part-way through skipping an attribute.

As for a testcase, the example you give above doesn't get here. Perhaps something like:

int *p;
void f(float *q) {
  float(* [[attr([])]] p);
  p = q; // check we disambiguated as a declaration not an expression
}

802

Because SkipUntil handles nested parens, this does the right thing for __attribute__((...)). Agreed on test coverage :)

811–813

We're missing _Atomic from this list. Eg:

void f() {
  int (*_Atomic p);
}

... is incorrectly disambiguated as an expression.

[aaron.ballman]

clang/lib/Parse/ParseTentative.cpp

786

Okay, that's a good point. Thanks!

802

I'll update the docs for SkipUntil(), because they don't suggest that this magic takes place. :-)

[michele.scandale]

Tests for __attribute__(()) syntax + attribute arguments + fix for _Atomic qualifier.

[aaron.ballman]

Thank you for the new tests and drive-by fix for _Atomic! LGTM

[aaron.ballman]

Thank you for the patch -- I've commit on your behalf in bd5b22070b6984d89c13b6cf38c3e54fc98ce291

clang/lib/Parse/ParseTentative.cpp

if (IsAttribute)
     return CAK_AttributeSpecifier;
 
   // Case 3: Message send.
   return CAK_NotAttributeSpecifier;
 }
 
 Parser::TPResult Parser::TryParsePtrOperatorSeq() {
   while (true) {
     if (TryAnnotateOptionalCXXScopeToken(true))
       return TPResult::Error;
 
     if (Tok.isOneOf(tok::star, tok::amp, tok::caret, tok::ampamp) ||
         (Tok.is(tok::annot_cxxscope) && NextToken().is(tok::star))) {
       // ptr-operator
       ConsumeAnyToken();
+
+      // Skip attributes.
+      while (Tok.isOneOf(tok::l_square, tok::kw___attribute, tok::kw___declspec,
+                         tok::kw_alignas)) {
+        if (Tok.is(tok::l_square)) {
+          ConsumeBracket();
+          if (!SkipUntil(tok::r_square))
+            return TPResult::Error;
+        } else {
+          ConsumeToken();
+          if (Tok.isNot(tok::l_paren))
+            return TPResult::Error;
+          ConsumeParen();
+          if (!SkipUntil(tok::r_paren))
+            return TPResult::Error;
+        }
+      }
+
       while (Tok.isOneOf(tok::kw_const, tok::kw_volatile, tok::kw_restrict,
                          tok::kw__Nonnull, tok::kw__Nullable,
                          tok::kw__Null_unspecified))
         ConsumeToken();
     } else {
       return TPResult::True;
     }
   }
 }
 
 ///         operator-function-id:

clang/test/CXX/dcl.decl/p4-0x.cpp

 // RUN: %clang_cc1 -std=c++11 -fsyntax-only -verify %s
-// expected-no-diagnostics
 
 struct X {
   void f() &;
   void g() &&;
 };
 
 void (X::*pmf)() & = &X::f;
+
+void fn() {
+  void (*[[attr]] fn_ptr)() = &fn; // expected-warning{{unknown attribute 'attr' ignored}}
+  void (*[[attrA]] *[[attrB]] fn_ptr_ptr)() = &fn_ptr; // expected-warning{{unknown attribute 'attrA' ignored}} expected-warning{{unknown attribute 'attrB' ignored}}
+
+  void (&[[attr]] fn_lref)() = fn; // expected-warning{{unknown attribute 'attr' ignored}}
+  void (&&[[attr]] fn_rref)() = fn; // expected-warning{{unknown attribute 'attr' ignored}}
+}