[PATCH] D75844: [clang] Set begin loc on GNU attribute parsed attrs

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D75844#2076260 <https://reviews.llvm.org/D75844#2076260>, @tbaeder wrote:

> I'm looking at this again and I am not sure how it is meant to work. For example in `Parser::parseClassSpecifier` in `ParseDeclCXX.cpp`.
>  Here `attrs` is a local variable of type `ParsedAttributesWithRange`, already before my patch. `attrs` is then passed to
>
> 1. `MaybeParseGNUAttributes`
> 2. `MaybeParseMicrosoftDeclSpecs`
> 3. `ParseMicrosoftInheritanceClassAttributes`
> 4. `MaybeParseCXX11Attributes`
>
>   and later `parseClassSpecifier` calls `ProhibitAttributes(attrs)` a few times. `ProhibitAttributes` in turn will not do anything if the given attrs have an invalid (i.e. unset) range. So, how could they ever have a valid range set? All the four functions above only take a  `ParsedAttributes`, no range.
>
>   This is one of the cases that now (that `MaybeParseGNUAttributes` sets the range of the given `attrs` if it really parses attributes) generates errors in various test cases. For example in `clang/test/AST/ast-print-record-decl.c`: File /home/tbaeder/llvm-project/clang/test/AST/ast-print-record-decl.c Line 209: an attribute list cannot appear here
>
>   Am I missing something?


I don't think you're missing anything -- the state of the system is currently inconsistent. From (possibly faulty) memory, we previously only tracked the minimum of location information for attribute source locations and some parts of the system relied on invalid source locations to convey meaning that it shouldn't have. This is compounded by the fact that the different attribute syntaxes (gnu, C++, declspec, etc) have been worked on at different times with different assumptions about source locations, but are all generalized in the same parsed attributes data structure. Now that you're starting to more consistently track source location information across syntaxes, you're hitting some fallout from those inconsistencies. Hopefully there's not so much fallout that it cannot be handled though (if there is, please speak up and we can try to explore other options).


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

https://reviews.llvm.org/D75844