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

[Nick Desaulniers via Phabricator via cfe-commits]

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