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