AaronBallman wrote: > > > Yeah, I think I can live with this. I think not supporting the arbitrary > > > order is more annoying to users than a slight degradation in source > > > location reporting. > > > > > > The point @kadircet brings up about tooling is a good one, though. I'm not > > certain we need to revert the patch, but breaking a bunch of tools and > > making them cope with the source location change is pretty disruptive. I > > think we may need to consider refactoring source location handling for > > attributes more broadly to solve the underlying concerns. Thoughts > > @erichkeane? > > Related, this issue just was filed today: #140020 > > I wouldn't be opposed to SOME sort of improvement in attributes handling. We > flatten the list of attributes unfortunately, so we couldn't add source > location to each group in any way.... > > My one immediate thought is: What if we added ONE extra source-location to > `AttrCommonInfo`? The FIRST in each group gets the 'begin' location, and the > 'LAST' in each group gets the 'end' location. Everyone else gets an empty > source location. We could make it private, then just accessible via functions > from Decl. Since we maintain order of attributes, we'd get them reasonably > well. > > WDYT?
I think that's still going to be too clever for tooling because tools will do stuff like use AST matchers to say "does this declaration have this attribute? Cool, let's add a fix-it to remove that range of characters." and that's going to fail for them when the attribute is middle in a list of three. I think what we need is: 1) Each attribute stores the source range for that one attribute. The beginning of the range is the attribute token itself. The end of the range is either closed when there's no argument list or it's the closing paren for the attribute argument list. 2) We introduce a helper method which gets you the full source range of all attributes within one syntactic group for a declaration. e.g., `[[foo, bar(12), baz]]`; each attribute tracks its source information, and the helper function gets you the range starting from `[` and ending with `]`. 3) We introduce another helper method with gets you the full source range of all attributes within all syntactic groups for a declaration. e.g., `[[foo, bar]] __attribute__((baz(12))) __declspec(no_bad)`; each attribute tracks its source information, we use the helper from (2) to get the range for each syntax group, and we extend the returned range to cover those ranges. In this example, the start of the range is `[` and the end of the range is `)` from the `__declspec`. I think this ends up getting full fidelity for tools to use without having to store too many locations, but the problem with (2) and (3) is that we still need those helpers to be smart about inherited attributes. e.g., ``` __attribute__((foo)) void func(); [[bar]] void func(); // Inherits __attribute__((foo)) ``` so those helpers really only make sense on a single declaration, not for inherited attributes. WDYT? > I don't really have the time/ability to do so, but am definitely willing to > review something like this. I'm in the same boat. https://github.com/llvm/llvm-project/pull/133107 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
