Page MenuHomePhabricator

[clang] source range of variable template specialization should include initializer
ClosedPublic

Authored by tomasz-kaminski-sonarsource on Mar 23 2023, 9:39 AM.

Details

Summary

This patch adjust the getSourceRange() for the
VarTemplateSpecializationDecl and VarTemplatePartialSpecializationDecl,
such that the initializer is included if present:

template<typename T>
T temp = 1;

template<> double temp<double> = 1;

This patch makes it consistent with the behavior of
non-template variables with initializers and restores
behavior that was present before https://reviews.llvm.org/D139705.

n case, when the initializer is not present we still
include the template arguments in the source range,
which was required for fixing zero-initialization fix-it.

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptMar 23 2023, 9:40 AM
tomasz-kaminski-sonarsource requested review of this revision.Mar 23 2023, 9:40 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 23 2023, 9:40 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

This needs a test and a release note. Patch otherwise looks fine to me.

Comments no longer misleading reffer to fallback to DeclatorDecl

erichkeane added inline comments.Mar 23 2023, 9:48 AM
clang/lib/AST/DeclTemplate.cpp
1460

Looking closer... VarDecl already includes its init in its source range, right? So perhaps all that needs to happen here is to have the isExplicitSpecialization check be :

isExplicitSpecialization() && !hasInit()

WDYT?

This needs a test and a release note. Patch otherwise looks fine to me.

Do you have any pointers for a test of source ranges? The patch that caused regression used a fix-it hit, however, it is not useful in this case, as not fixes are raised in the case when the initializer is present.

Just noticed your commit-notice reference to the 'breaking' change is incorrect, should be:
https://reviews.llvm.org/D139705

As for the test, an AST-Dump test has the line/column info in it, so that should be a good enough test.

tomasz-kaminski-sonarsource edited the summary of this revision. (Show Details)

Simpified condition

I have updated the description and implementation. Will look at adding a unit test tomorrow.

clang/lib/AST/DeclTemplate.cpp
1460

Great suggestion. I really disliked the repetition.

Test looks fine, still need a release note.

Including release note

erichkeane added inline comments.Mar 24 2023, 6:56 AM
clang/docs/ReleaseNotes.rst
231

Needs a little more detail here. Something about what was wrong before, and what is wrong now, particularly since there is no github bug to link to.

Updated release note to be more descriptive.

erichkeane accepted this revision.Mar 24 2023, 7:11 AM

LGTM! Let me know if you need someone to commit this for you, and include "Name To Use <EmailAddr>"

This revision is now accepted and ready to land.Mar 24 2023, 7:11 AM

Rebasing on test commit. My applogies for noise, I still get confused with arc sometimes.

LGTM! Let me know if you need someone to commit this for you, and include "Name To Use <EmailAddr>"

I have commit rights, but I will not be able to commit this before Monday.
I someone would like to commit these two patches, which I would appreciate, please use "Tomasz Kamiński <tomasz.kaminski@sonarsource.com>".

This revision was landed with ongoing or failed builds.Mar 27 2023, 3:37 AM
This revision was automatically updated to reflect the committed changes.