nickdesaulniers 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) {
----------------
aaron.ballman wrote:
> 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!
> now that I see how my suggestion is playing out in practice
> Thank you for trying this out, sorry for the hassle!
Ah, that's ok. [[ https://www.youtube.com/watch?v=aPVLyB0Yc6I | Sometimes you
eat the bar, sometimes the bar eats you. ]] I've done that myself already in
this patch when playing with llvm::Optional (and C++ default parameters).
Changed back to string comparison now. I don't expect any of this code to be
hot, ever. Marking as done.
================
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
----------------
aaron.ballman wrote:
> 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.
Since I got the idea for this feature from -Wframe-larger-than=, I followed
clang/test/Frontend/backend-diagnostic.c, both for the REQUIRES line and the
test location.
I guess clang/test/Frontend/backend-diagnostic.c uses x86 inline asm, so maybe
I can remove the REQUIRES line. But if my new tests move then so should
clang/test/Frontend/backend-diagnostic.c (maybe pre-commit). Will leave that
for other reviewers, but removing REQUIRES lines.
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
