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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
