[PATCH] D74643: [Parse] Consider attributes in `TryParsePtrOperatorSeq`.

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.


================
Comment at: clang/lib/Parse/ParseTentative.cpp:773
+  while (Tok.isOneOf(tok::l_square, tok::kw___attribute, tok::kw___declspec,
+                     tok::kw_alignas)) {
+    if (Tok.is(tok::l_square)) {
----------------
aaron.ballman wrote:
> Do we need to also skip other keyword attributes as well as `alignas`? For instance, `tok::kw__Noreturn` or `tok::kw__Alignas`
`_Alignas` and `_Noreturn` are their own kinds of //decl-specifier//s, 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.


================
Comment at: clang/lib/Parse/ParseTentative.cpp:779
+      ConsumeBracket();
+      if (!SkipUntil(tok::r_square) || Tok.isNot(tok::r_square))
+        return false;
----------------
aaron.ballman wrote:
> 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.
`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
}
```


================
Comment at: clang/lib/Parse/ParseTentative.cpp:789
+      ConsumeParen();
+      if (!SkipUntil(tok::r_paren))
+        return false;
----------------
aaron.ballman wrote:
> 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.
Because `SkipUntil` handles nested parens, this does the right thing for `__attribute__((...))`. Agreed on test coverage :)


================
Comment at: clang/lib/Parse/ParseTentative.cpp:811-813
       while (Tok.isOneOf(tok::kw_const, tok::kw_volatile, tok::kw_restrict,
                          tok::kw__Nonnull, tok::kw__Nullable,
                          tok::kw__Null_unspecified))
----------------
We're missing `_Atomic` from this list. Eg:

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

... is incorrectly disambiguated as an expression.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74643