aaron.ballman added inline comments.
================
Comment at: clang/lib/CodeGen/CodeGenAction.cpp:783
+ : diag::warn_fe_backend_warning_attr)
+ << FD->getDeclName() << EA->getUserDiagnostic();
+ }
----------------
The diagnostics engine knows how to properly format a `NamedDecl *` directly
(and this will take care of properly quoting the name in the diagnostics as
well).
================
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:
> > 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.
Thanks!
================
Comment at: clang/test/CodeGen/attr-error.c:9
+// CHECK: call void @foo(), !srcloc [[SRCLOC:![0-9]+]]
+// CHECK: declare void @foo() [[ATTR:#[0-9]+]]
+// CHECK: attributes [[ATTR]] = {{{.*}}"dontcall"
----------------
This is failing the CI pipeline on Windows because the declaration there looks
like: `declare dso_local void @foo() #1`
================
Comment at: clang/test/CodeGen/attr-warning.c:9
+// CHECK: call void @foo(), !srcloc [[SRCLOC:![0-9]+]]
+// CHECK: declare void @foo() [[ATTR:#[0-9]+]]
+// CHECK: attributes [[ATTR]] = {{{.*}}"dontcall"
----------------
Similar issue here as above.
================
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
----------------
nickdesaulniers wrote:
> 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.
Given that there's not an obviously correct answer, I'm fine leaving the tests
here. We can rearrange the deck chairs a different day if we'd like.
================
Comment at: clang/test/Sema/attr-error.c:26
+
+__attribute__((error("foo"), warning("foo"))) // expected-error {{'warning'
and 'error' attributes are not compatible}}
+int
----------------
Can you also add a test for:
```
__attribute__((error("foo"))) int bad5(void); // expected-note {{conflicting
attribute is here}}
__attribute__((warning("foo"))) int bad5(void); // expected-error {{'warning'
and 'error' attributes are not compatible}}
```
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
