aaron.ballman 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"
----------------
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?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D139705/new/
https://reviews.llvm.org/D139705
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
