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