[PATCH] D106030: [Clang] add support for error+warning fn attrs
Nick Desaulniers via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Fri Aug 6 14:59:16 PDT 2021
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
More information about the cfe-commits
mailing list