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
