[PATCH] D106030: [Clang] add support for error+warning fn attrs

[Aaron Ballman via Phabricator via cfe-commits]

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