[PATCH] D139705: [clang] fix zero-initialization fix-it for variable template

[Erich Keane via Phabricator via cfe-commits]

erichkeane accepted this revision.
erichkeane added inline comments.


================
Comment at: clang/test/FixIt/fixit-const-var-init.cpp:24
+
+template <> constexpr float d<int, float>; // expected-error {{must be initialized by a constant expression}}
+// CHECK: fix-it:"{{.*}}":{24:42-24:42}:" = 0.0"
----------------
aaron.ballman wrote:
> v1nh1shungry wrote:
> > aaron.ballman wrote:
> > > v1nh1shungry wrote:
> > > > aaron.ballman wrote:
> > > > > I'd like to see some additional test coverage for more complicated declarators:
> > > > > ```
> > > > > void (* const func)(int, int);
> > > > > const int var [[clang::annotate("")]];
> > > > > ```
> > > > One more question: I just did the second test
> > > > 
> > > > ```
> > > > const int var [[clang::annotate("")]];
> > > > ```
> > > > 
> > > > and it inserted the fix hint after the identifier.
> > > > 
> > > > Is this the expected behavior of `getEndLoc()`?
> > > @erichkeane and I talked about this off-list because it's a bit of an academic question as to whether an attribute is "part" of a declaration or not. We ultimately decided that we think it's best for the attribute to *not* be considered part of the source range of the variable declaration; they are wholly separate AST nodes.
> > > 
> > > So based on that, we think you will need some changes to SemaInit.cpp after all. And you'll have to use `SourceManager::isBeforeInTranslationUnit()` to determine if the begin source location of the attribute is after the end source location of the variable or not (because the attribute could go before the type information or after the variable name).
> > > 
> > > There's another test case to consider that hopefully will Just Work with this design, but is worth testing explicitly:
> > > ```
> > > void (* const func [[clang::annotate("test")]])(int, int);
> > > ```
> > I have trouble getting the location of the right bracket of the attribute list, which may be used to insert the fix hint. Could you please give me some hints? Thanks a lot!
> > I have trouble getting the location of the right bracket of the attribute list, which may be used to insert the fix hint. Could you please give me some hints? Thanks a lot!
> 
> Oh boy, that's an interesting problem, isn't it! We keep all of the attribute AST nodes with the declaration, and each attribute knows its own source range, but we *don't* keep the source locations for where the full attribute list appears. e.g.,
> ```
> __attribute__((foo)) const int var [[bar, baz]];
> ```
> `var` will have three attributes associated with it, but the only source location information you have access to is for the `foo`, `bar`, and `baz` tokens. Each of those attributes also keeps track of what syntax was used, so you could tell that `foo` was a GNU-style attribute while `bar` and `baz` were C++. But we don't keep enough information about `bar` and `baz` being part of the same attribute list or whether that attribute list is leading or trailing. You have to calculate all of this yourself and some of it might not even be possible to calculate (for example, the user could have done `__attribute__((foo)) const int var [[bar]] [[baz]];` and the AST would come out the same as the previous example).
> 
> I'd say that's way more work than is reasonable for this patch, so my suggestion is to file a GitHub issue about the problem with attributes, note the implementation issues, and then we'll ignore attributes for this patch.
> 
> @erichkeane -- something for us to consider is how to improve this in the AST. We run into problems when doing AST pretty printing because of attributes for these same reasons. It seems to me that the `Decl`/`Stmt`/`TypeLoc` nodes need to keep a list of source ranges for attributes around (since there can be multiple ranges for declarations). WDYT?
I took conversation to the bug here: https://github.com/llvm/llvm-project/issues/59935


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139705