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