aaron.ballman accepted this revision.
aaron.ballman added a comment.

LGTM!



================
Comment at: clang/include/clang/Basic/AttributeCommonInfo.h:153
     // FIXME: Use a better mechanism to determine this.
+    // We use this in `isCXX11Attribute` below, so it _should_ only return
+    // true for the `alignas` spelling, but it currently also returns true
----------------
mboehme wrote:
> aaron.ballman wrote:
> > Now that C23 has attributes, it's also not clear whether `_Alignas` will be 
> > handled slightly differently in the future as an attribute rather than a 
> > declaration specifier as happened with `_Noreturn` for C23. So agreed this 
> > is a tricky area.
> Thanks for pointing this out! Is there anything specific I should add to the 
> code comment, or is your comment just for general awareness?
Nope, just spreading the information around to more people's brains -- nothing 
needs to be changed here.


================
Comment at: clang/include/clang/Sema/ParsedAttr.h:1126
+/// `Result`. Sets `Result.Range` to the combined range of `First` and 
`Second`.
+void ConcatenateAttributes(ParsedAttributes &First, ParsedAttributes &Second,
+                           ParsedAttributes &Result);
----------------
mboehme wrote:
> aaron.ballman wrote:
> > I think `Concatenate` implies that `First` and `Second` are untouched, but 
> > that's not really the case here. Perhaps `takeAndConcatenateAll()` or 
> > something along those lines instead? Also, the function should start with a 
> > lowercase letter per the usual coding style rules.
> Good point. Done, with a slightly different name -- WDYT?
Works great for me!


================
Comment at: clang/lib/Parse/ParseStmt.cpp:229
   default: {
+    bool HaveAttrs = !(CXX11Attrs.empty() && GNUAttrs.empty());
+    auto IsStmtAttr = [](ParsedAttr &Attr) { return Attr.isStmtAttr(); };
----------------
I don't insist, but I think it's a tiny bit easier to read this way.


================
Comment at: clang/lib/Parse/ParseStmt.cpp:321-323
+    for (const ParsedAttr &AL : CXX11Attrs) {
+      Diag(AL.getRange().getBegin(), diag::warn_attribute_ignored) << AL;
+    }
----------------



================
Comment at: clang/lib/Parse/ParseStmt.cpp:230-237
     if ((getLangOpts().CPlusPlus || getLangOpts().MicrosoftExt ||
          (StmtCtx & ParsedStmtContext::AllowDeclarationsInC) !=
              ParsedStmtContext()) &&
         ((GNUAttributeLoc.isValid() &&
-          !(!Attrs.empty() &&
-            llvm::all_of(
-                Attrs, [](ParsedAttr &Attr) { return Attr.isStmtAttr(); }))) ||
+          !(!(CXX11Attrs.empty() && GNUAttrs.empty()) &&
+            llvm::all_of(CXX11Attrs, isStmtAttr) &&
+            llvm::all_of(GNUAttrs, isStmtAttr))) ||
----------------
mboehme wrote:
> aaron.ballman wrote:
> > The logic is correct here, but this predicate has gotten really difficult 
> > to understand. If you wanted to split some of those conditions out into 
> > named variables for clarity, I would not be sad (but I also don't insist 
> > either).
> Yes, this is confusing. I've factored out some variables that I hope make 
> this more readable.
> 
> By the way, it might look as if `GNUAttributeLoc.isValid()` implies 
> `HaveAttributes` and that the latter is therefore redundant, but this isn’t 
> actually true if we failed to parse the GNU attribute. Removing the 
> `HaveAttributes` makes some tests fail, e.g. line 78 of 
> clang/test/Parser/stmt-attributes.c.
Thanks, I think this is more readable this way!


================
Comment at: clang/lib/Parse/ParseStmt.cpp:248-251
+      if (CXX11Attrs.Range.getBegin().isValid())
+        DeclStart = CXX11Attrs.Range.getBegin();
+      else if (GNUAttrs.Range.getBegin().isValid())
+        DeclStart = GNUAttrs.Range.getBegin();
----------------
mboehme wrote:
> aaron.ballman wrote:
> > Do CXX attributes always precede GNU ones, or do we need to do a source 
> > location comparison here to see which location is lexically earlier?
> Yes, we can rely on this being the case. This function is called from only 
> one place where both CXX11Attrs and GNUAttrs can potentially contain 
> attributes, namely `ParseStatementOrDeclaration()` (line 114 in this file). 
> There, the CXX11Attrs are parsed before the GNUAttrs. I’ve added an 
> assertion, however, since this guarantee is important to the correctness of 
> the code here.
Thank you for the confirmation and the added assertion!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D126061/new/

https://reviews.llvm.org/D126061

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to