aaron.ballman added inline comments.
================
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) {
----------------
nickdesaulniers wrote:
> 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?
> (I guess it's not clear to me what's CXX11_gnu_* vs C2x_gnu_*?)
A bit of a historical thing. C2x `[[]]` and C++11 `[[]]` are modeled as
different spellings. This allows us to specify attributes with a `[[]]`
spelling in only one of the languages without having to do an awkward dance
involving language options. e.g., it makes it easier to support an attribute
spelled `__attribute__((foo))` and `[[vendor::foo]]` in C2x and spelled
`[[foo]]` in C++. it picks up the `gnu` bit in the spelling by virtue of being
an attribute with a `GCC` spelling -- IIRC, we needed to distinguish between
GCC and Clang there for some reason I no longer recall.
> WDYT?
I think the current approach is reasonable, but I think the previous approach
(doing the string compare) was more readable. If you have a preference for
using the string compare approach as it originally was, I'd be fine with that
now that I see how my suggestion is playing out in practice. If you'd prefer to
keep the current approach, I'd also be fine with that. Thank you for trying
this out, sorry for the hassle!
================
Comment at: clang/test/Frontend/backend-attribute-error-warning-optimize.c:1
+// REQUIRES: x86-registered-target
+// RUN: %clang_cc1 -O2 -verify -emit-codegen-only %s
----------------
Out of curiosity, why is this required? I would have assumed this would be
backend-independent functionality?
Also, I have no idea where these tests should live. They're not really frontend
tests, it's more about the integration between the frontend and the backend. We
do have `clang/test/Integration` but there's not a lot of content there to be
able to say "oh yeah, this is testing the same sort of stuff". No idea if other
reviewers have opinions on this.
================
Comment at: clang/test/Frontend/backend-attribute-error-warning.c:3
+// RUN: %clang_cc1 -verify -emit-codegen-only %s
+// RUN: %clang_cc1 -verify -emit-codegen-only -Wno-attribute-warning %s
-DWARNINGS_DISABLED
+
----------------
A different approach to using the preprocessor is to use give a prefix to
`-verify`. e.g., on the previous RUN line, you can do: `-verify=expected,
enabled` and on this run line you can do `-verify`. Then, when you have an
optional warning, you can use `// enabled-warning {{whatever}}` and it'll be
checked only for the first RUN line due to the `-verify` arguments.
================
Comment at: clang/test/Frontend/backend-attribute-error-warning.cpp:2-3
+// REQUIRES: x86-registered-target
+// RUN: %clang_cc1 -verify -emit-codegen-only %s
+// RUN: %clang_cc1 -verify -emit-codegen-only -Wno-attribute-warning %s
-DWARNINGS_DISABLED
+
----------------
Given how similar this is to the .c test, I'd recommend merging the contents of
the tests together with an additional RUN line with `-x c++` to compile in C++
mode and using `#if defined(__cplusplus)` for the very small amount of
C++-specific stuff being tested. 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
