[PATCH] D126061: [clang] Reject non-declaration C++11 attributes on declarations

[Aaron Ballman via Phabricator via cfe-commits]

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