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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
