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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D139705#4216449 <https://reviews.llvm.org/D139705#4216449>, @tomasz-kaminski-sonarsource wrote:

> In D139705#4216417 <https://reviews.llvm.org/D139705#4216417>, @erichkeane wrote:
>
>> In D139705#4215653 <https://reviews.llvm.org/D139705#4215653>, @tomasz-kaminski-sonarsource wrote:
>>
>>> 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.
>>
>> 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?
>
> Before the change, 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 inconsistent

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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139705