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

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

In D139705#4216480 <https://reviews.llvm.org/D139705#4216480>, @aaron.ballman wrote:

> 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?

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139705