compnerd marked 5 inline comments as done.
compnerd added inline comments.
================
Comment at: clang/include/clang/Parse/Parser.h:1605-1606
// C99 6.9: External Definitions.
DeclGroupPtrTy ParseExternalDeclaration(ParsedAttributes &Attrs,
+ ParsedAttributes &DeclSpecAttrs,
ParsingDeclSpec *DS = nullptr);
----------------
aaron.ballman wrote:
> Should we rename `Attrs` to `DeclaratorAttrs` or something along those lines,
> so it's easier for someone to distinguish which attributes go where? (Same
> suggestion applies throughout the review.)
Yes, `DeclAttrs` would be much nicer, but I wanted to get everything right
before doing that and then forgot. Thanks for the reminder.
================
Comment at: clang/lib/Parse/Parser.cpp:1096-1097
+ ParsingDeclSpec &DS, AccessSpecifier AS) {
+ DS.SetRangeStart(DeclSpecAttrs.Range.getBegin());
+ DS.SetRangeEnd(DeclSpecAttrs.Range.getEnd());
+ DS.takeAttributesFrom(DeclSpecAttrs);
----------------
aaron.ballman wrote:
> How sure are you that this isn't overwriting existing range data that's
> potentially relevant? e.g., where declaration specifiers and attributes are
> mixed together such that the attribute range doesn't cover all of the
> declaration specifiers?
I don't believe it can - the range hasn't been setup yet when coming into this
function, only the storage has been allocated. This is the first place where
we are initializing the source range. If there are other unstated invariants,
it would be nice to have some sort of assertion for them.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D137979/new/
https://reviews.llvm.org/D137979
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
