nickdesaulniers added inline comments.
================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:11172
+def err_attribute_removed_on_redeclaration : Error<
+ "'%0' attribute removed from redeclared variable">;
----------------
aaron.ballman wrote:
> This diagnostic is a bit confusing to me -- should this be a warning about
> the attribute being ignored in this case, rather than an error?
> Alternatively, should this be re-worded to say that the attribute must appear
> on the first declaration? If the latter, we have some diagnostics that could
> maybe be combined into using a `%select` for this:
> `err_noreturn_missing_on_first_decl` and `err_internal_linkage_redeclaration`
> are awfully similar to the same thing.
Rebased onto D107613; reusing `diag::err_attribute_missing_on_first_decl` now.
done.
================
Comment at: clang/lib/Sema/SemaDecl.cpp:2982
+ // If redeclaration removes error or warning attribute...
+ if (auto *EA = New->getAttr<ErrorAttr>())
----------------
aaron.ballman wrote:
> The comment doesn't seem to match the code -- this is when the redeclaration
> *adds* the attribute. And don't we want to silently accept that if the
> redeclaration adds the same annotation with the same message? e.g.,
> ```
> __attribute__((error("derp"))) void func();
> __attribute__((error("derp"))) void func(); // fine
> __attribute__((error("derp"))) void func() {} // fine
> ```
Rebased onto D107613, so this block was moved from `Sema::mergeDeclAttributes`
(which should be for `Attributes` on any subclass of `Decl`, which is too
generic for a `FunctionDecl` only `Attribute`) to `Sema::MergeFunctionDecl`
(which is only for `FunctionDecl` `Attribute`s).
I had a similar test case already in clang/test/Sema/attr-error.c testing
`__attribute__((error("foo"), error("foo"))) int good1(void);`, but I will add
a test to that file for redeclarations with matching attributes. done.
================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:955
+
+ if (const auto *EA = D->getAttr<ErrorAttr>()) {
+ unsigned NewAttrSpellingIndex = AL.getAttributeSpellingListIndex();
----------------
aaron.ballman wrote:
> This logic will catch the case where the attributes are processed in a group
> (e.g., two attributes in one specifier or two attributes on the same
> declaration) but it won't handle the redeclaration merging case. I think you
> need to follow the attribute merge pattern used in SemaDecl.cpp.
I'm not sure that I need to change anything here; redeclarations of
`ErrorAttr`s are checked in `Sema::MergeFunctionDecl`.
But when I was playing around with something else here earlier this week, I
noticed a pre-existing inconsistency (or maybe just a difference, described
below) of `handle*Attr` functionns vs `merge*Attr` functions.
`mergeDeclAttribute` in `clang/lib/Sema/SemaDecl.cpp` has a large `if`/`else`
chain for new attrs, which calls `merge*Attr` methods of `Sema`, but it doesn't
give your a reference to any "`Old`"/pre-existing attr that may be in conflict.
A comment in `mergeDeclAttribute` seems to confirm this.
>> // This function copies an attribute Attr from a previous declaration to the
>>
>>
>> // new declaration D if the new declaration doesn't itself have that
>> attribute
>>
>> // yet or if that attribute allows duplicates.
Because we do not allow this `Attribute` to be added on subsequent `Decl`s, I
_do not think_ we need to follow the attribute merge pattern used in
SemaDecl.cpp. But I could be wrong; please triple check.
================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:964-965
+ bool match =
+ (EA->isError() && NewAttrSpellingIndex < ErrorAttr::GNU_warning) ||
+ (EA->isWarning() && NewAttrSpellingIndex >= ErrorAttr::GNU_warning);
+ if (!match) {
----------------
aaron.ballman wrote:
> It's a bit tricky for me to tell from the patch -- are the enumerators in the
> correct order that this logic still works for when the [[]] spelling is used?
For reference, the generated `enum Spelling` looks like:
```
3611 public:
3612 enum Spelling {
3613 GNU_error = 0,
3614 CXX11_gnu_error = 1,
3615 C2x_gnu_error = 2,
3616 GNU_warning = 3,
3617 CXX11_gnu_warning = 4,
3618 C2x_gnu_warning = 5,
3619 SpellingNotCalculated = 15
3620
3621 };
```
while using `getAttributeSpellingListIndex` rather than `getNormalizedFullName`
allows us to avoid a pair of string comparisons in favor of a pair of
`unsigned` comparisons, we now have an issue because there are technically six
spellings. (I guess it's not clear to me what's `CXX11_gnu_*` vs `C2x_gnu_*`?)
We could be explicit against checking all six rather than two comparisons.
Or I can use local variables with more helpful identifiers like:
```
bool NewAttrIsError = NewAttrSpellingIndex < ErrorAttr::GNU_warning;
bool NewAttrIsWarning = NewAttrSpellingIndex >= ErrorAttr::GNU_warning;
bool Match = (EA->isError() && NewAttrIsError) || (EA->isWarning() &&
NewAttrIsWarning);
```
WDYT?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D106030/new/
https://reviews.llvm.org/D106030
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
