Current version there is a fix-it for
template <class> constexpr int x = 0; template <> constexpr int x<int>; // fix-it here
but it will cause
template <> constexpr int x = 0<int>;
Details
Details
Diff Detail
Diff Detail
- Repository
- rG LLVM Github Monorepo
Unit Tests
Unit Tests
Event Timeline
| clang/lib/Sema/SemaInit.cpp | ||
|---|---|---|
| 3870–3872 | If I understand correctly we either have a VarTemplatePartialSpecializationDecl or ASTTemplateArgumentListInfo so we should use an else if and if we do then we should add a brace to make the else if consistent with the else | |
Comment Actions
As far as teh fix itself, I think this is fine. BUT i think there is value in waiting for Aaron to see if there is a deeper issue here.
| clang/lib/Sema/SemaInit.cpp | ||
|---|---|---|
| 3863 | Hmm... it is strange to me that the variables 'endloc' is the end of the identifier and not the end of the variable declaration. I unfortunately don't have a good feeling as to the 'correct' behavior for that (Aaron is typically the one who understands source locations better than me!), so hopefully he can come along and see. | |
| clang/lib/Sema/SemaInit.cpp | ||
|---|---|---|
| 3863 | I don't think we should have to do this dance here, this is something that getEndLoc() should be able to tell us. The way that source locations work is that AST nodes can override getSourceRange() to produce the correct range information for the node, and then they expose different accessors for any other source location information. In this case, I think VarTemplateSpecializationDecl isn't overloading getSourceRange() and so we're getting the default behavior from VarDecl. | |
| clang/test/FixIt/fixit-const-var-init.cpp | ||
| 25 | I'd like to see some additional test coverage for more complicated declarators: void (* const func)(int, int);
const int var [[clang::annotate("")]]; | |
| clang/lib/Sema/SemaInit.cpp | ||
|---|---|---|
| 3863 | Sorry! I'm confused here. Do you mean we should use getEndLoc() directly as the location where the fix-hint insert? But isn't the original code using getEndLoc(), it turns out to add the fix-hint after the end of the identifier instead of the right angle of VarTemplateSpecializationDecl. | |
| clang/lib/Sema/SemaInit.cpp | ||
|---|---|---|
| 3863 | Right, we should use getEndLoc directly for hte fixit location, and 'fix' getEndLoc by having VarTemplateSpecializationDecl override getSourceRange to have the right end loc. | |
| clang/lib/Sema/SemaInit.cpp | ||
|---|---|---|
| 3863 |
Thanks for asking for clarification! I think the code here in SemaInit.cpp is correct as-is and we should instead be implementing VarTemplateSpecializationDecl::getSourceRange() to calculate the correct begin and end location for the variable declaration. This should fix SemaInit.cpp because getEndLoc() is implemented by calling getSourceRange() and returning the end location: https://github.com/llvm/llvm-project/blob/main/clang/include/clang/AST/DeclBase.h#L428 | |
| clang/lib/Sema/SemaInit.cpp | ||
|---|---|---|
| 3863 | I see. Thanks for the clarification! | |
| clang/test/FixIt/fixit-const-var-init.cpp | ||
|---|---|---|
| 25 | 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()? | |
| clang/test/FixIt/fixit-const-var-init.cpp | ||
|---|---|---|
| 25 | @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); | |
| clang/test/FixIt/fixit-const-var-init.cpp | ||
|---|---|---|
| 25 | 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! | |
| clang/test/FixIt/fixit-const-var-init.cpp | ||
|---|---|---|
| 25 |
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? | |
Comment Actions
Thanks for the reply! I'll raise a GitHub issue.
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).
Can I post your reply there? I think it will help.
and then we'll ignore attributes for this patch.
Then I think this patch is ready for a review.
Comment Actions
Yes, absolutely!
and then we'll ignore attributes for this patch.
Then I think this patch is ready for a review.
Changes LGTM though you should add a release note to tell users about the fix. Thank you!
Comment Actions
Thank you for reviewing! I have opened an issue on GitHub: https://github.com/llvm/llvm-project/issues/59935. Hope my description is appropriate.
| clang/test/FixIt/fixit-const-var-init.cpp | ||
|---|---|---|
| 25 | I took conversation to the bug here: https://github.com/llvm/llvm-project/issues/59935 | |
Comment Actions
I'm happy to, what name and email address would you like used for patch attribution? Alternatively, it seems that you've had a few patches accepted in the project, so you could apply for commit access yourself if you'd like: https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access
Comment Actions
I'm happy to, what name and email address would you like used for patch attribution?
I'd like "v1nh1shungry" and "v1nh1shungry@outlook.com". Thank you!
Alternatively, it seems that you've had a few patches accepted in the project, so you could apply for commit access yourself if you'd like: https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access
Cool! I'll give it a try. Thanks!
Comment Actions
Sorry to disturb you again! It's been a week since I sent an email to Chris, and I have received no reply. Maybe my request was refused. It'd be great to land this and https://reviews.llvm.org/D140838 before the 16 release, right? If so, could you please help me commit them? I'd like "v1nh1shungry <v1nh1shungry@outlook.com>". Thank you so much!
Comment Actions
I doubt your request was refused, my guess is that Chris is busy at the moment (CC @lattner). I'm happy to land this for you though!
Comment Actions
I'm pretty sure I'm on top of commit access requests now, plz let me know if I missed you or something! Could be spam filter or who knows what
Comment Actions
I sent an email from <v1nh1shungry@outlook.com> to
<clattner@llvm.org> on last Friday, and I've received no reply so far. I've checked the email address many times so it was not likely to be sent to the wrong person.
I can resend the email. Or maybe we can deal with it here? If so, my GitHub username is v1nh1shungry (https://github.com/v1nh1shungry).
Thank you so much!
Comment Actions
As a downstream, we have concerns with this change. From what I saw it breaks the behavior of the fix-it that wants to remove the whole variable definition (including the initializer). For example, I have unused, that I want to remove variable x and unnecessary explicit specialization temp<double>:
template<typename T> T temp = 1; int x = 10; template<> double temp<double> = 1;
Previously, this could be handled (in case of single variable declaration) by simply removing up the var->getSourceRange() with succeeding coma. However, after the change, such code does not work, and in general if getSourceRange() is used on VarDecl (or even Decl), special consideration needs to be taken for the situation when VarDecl refers to variable template specialization.
As an alternative, I would suggest introducing an additional function to VarDecl (which could be moved up in the hierarchy), that would report a source range that corresponds to declarator-id, i.e. for template variable it would include template arguments.
Comment Actions
Hmm... I'm being a little dense here I guess, I don't understand what you mean? Does this result in one of our fixits being wrong here? With the old range, wouldn't it have left the <double> in that case, and now is removing it? Or what am I missing?
Comment Actions
For both x and temp<double>, prior to the change the getSourceRange() on the VarDecl that represents them includes an initializer (they end just before ;). But now for the variable template specialization, we end up just after template arguments. This means that fixit/report that previously covered the whole definition, will now not include an initializer.
Or in our example:
template<typename T>
T temp = 1;
// v getSourceRange() ends on this token before and after the change
int x = 10;
// v getSourceRange() ends on this token prior to the change, consistently with normal variables
template<> double temp<double> = 1;
// ^ getSourceRange() ends on this token after the change, making it inconsistentComment Actions
Thank you for the further explanation, that clarified the concern for me as well. I think I agree with you -- we used to cover the full source range for the AST node, and now we only cover part of it because we're missing the initializer. We want getSourceRange() to cover the full range of text for the node, so we should have a different function to access the more limited range. @v1nh1shungry is this something you'd feel comfortable fixing up?
Comment Actions
Since the motivation for this patch here was "make sure we're pointing to the 'end' so we can suggest an init fixit", perhaps this SHOULD be changed to the 'end' still, but just fix the case where the initializer was omitted. So
/// What USED to happen: template<> double temp<double> = 1; //End is here: ^ template<> double temp<double>; //End is here: ^ //What is happening now: template<> double temp<double> = 1; //End is here: ^ template<> double temp<double>; //End is here: ^ // What I think we want: template<> double temp<double> = 1; //End is here: ^ template<> double temp<double>; //End is here: ^
Right? So this isn't so much as a separate function, its just to make sure we get the 'source range' to include 'everything', including the initializer, if present.
Comment Actions
Since the motivation for this patch here was "make sure we're pointing to the 'end' so we can suggest an init fixit", perhaps this SHOULD be changed to the 'end' still, but just fix the case where the initializer was omitted. So
/// What USED to happen: template<> double temp<double> = 1; //End is here: ^ template<> double temp<double>; //End is here: ^ //What is happening now: template<> double temp<double> = 1; //End is here: ^ template<> double temp<double>; //End is here: ^ // What I think we want: template<> double temp<double> = 1; //End is here: ^ template<> double temp<double>; //End is here: ^Right? So this isn't so much as a separate function, its just to make sure we get the 'source range' to include 'everything', including the initializer, if present.
Indeed, this would address our concern, and allow properly inserting initializer. This would build down to repeating the condition from here: https://github.com/llvm/llvm-project/blob/bbe1394c9602ab9a939d9b17199d5f538cac9d0c/clang/lib/AST/Decl.cpp#L1207.
I was suggesting adding an additional function, as it would cover additional use cases like replacing or removing the initializer, and then VarDecl::getSourceRange could be defined as:
SourceRange VarDecl::getSourceRange() const {
if (const Expr *Init = getInit()) {
SourceLocation InitEnd = Init->getEndLoc();
// If Init is implicit, ignore its source range and fallback on
// DeclaratorDecl::getSourceRange() to handle postfix elements.
if (InitEnd.isValid() && InitEnd != getLocation())
return SourceRange(getOuterLocStart(), InitEnd);
}
return getDeclatorRange();
}Comment Actions
That would make sense to me. Feel free to submit a patch (assuming @v1nh1shungry doesn't respond/get to it first).
Comment Actions
Thank you for the suggestion! This makes sense to me too. But I'm sorry for being busy recently. Please feel free to submit a patch. Thank you!
Comment Actions
Indeed, this would address our concern, and allow properly inserting initializer. This would build down to repeating the condition from here: https://github.com/llvm/llvm-project/blob/bbe1394c9602ab9a939d9b17199d5f538cac9d0c/clang/lib/AST/Decl.cpp#L1207.
I was suggesting adding an additional function, as it would cover additional use cases like replacing or removing the initializer, and then VarDecl::getSourceRange could be defined as:
SourceRange VarDecl::getSourceRange() const { if (const Expr *Init = getInit()) { SourceLocation InitEnd = Init->getEndLoc(); // If Init is implicit, ignore its source range and fallback on // DeclaratorDecl::getSourceRange() to handle postfix elements. if (InitEnd.isValid() && InitEnd != getLocation()) return SourceRange(getOuterLocStart(), InitEnd); } return getDeclatorRange(); }That would make sense to me. Feel free to submit a patch (assuming @v1nh1shungry doesn't respond/get to it first).
I https://reviews.llvm.org/D146733 I have submitted a smaller patch, that addresses the regression and the concern. Unfortunately, at this time I am not able to commit to the introduction of the additional method, and its proper handling for AST nodes.
Comment Actions
While creating a test for the fix, I noticed the getSourceRange behavior is not stable after the serialization, as illustrated here: https://reviews.llvm.org/D146784
Hmm... it is strange to me that the variables 'endloc' is the end of the identifier and not the end of the variable declaration. I unfortunately don't have a good feeling as to the 'correct' behavior for that (Aaron is typically the one who understands source locations better than me!), so hopefully he can come along and see.