hokein added inline comments.
================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:225
tooling::Replacements FilteredChanges;
- for (const tooling::SymbolOccurrence &Rename :
- findOccurrencesWithinFile(AST, RenameDecl)) {
- // Currently, we only support normal rename (one range) for C/C++.
- // FIXME: support multiple-range rename for objective-c methods.
- if (Rename.getNameRanges().size() > 1)
- continue;
- // We shouldn't have conflicting replacements. If there are conflicts, it
- // means that we have bugs either in clangd or in Rename library, therefore
- // we refuse to perform the rename.
+ for (SourceLocation Loc : findOccurrencesWithinFile(AST, *RenameDecl)) {
+ // We shouldn't have conflicting replacements or replacements from
different
----------------
ilya-biryukov wrote:
> hokein wrote:
> > ilya-biryukov wrote:
> > > This seems to assume all occurrences are outside macros.
> > > Won't it break in presence of macros?
> > > Do we have tests when the renamed token is:
> > > - inside macro body
> > > - inside macro arguments
> > > ?
> > if Loc is a macro location, tooling::Replacement will use the spelling
> > location, so if the spelling location is in the main file, the code is
> > correct, we have test cases for it.
> >
> > but we will fail on cases like below, we should fix this, note that this
> > issue exists even before this patch. Updated the comment here.
> >
> > ```
> > // foo.h
> > #define MACRO foo
> >
> > // foo.cc
> > void f() {
> > int fo^o = 2;
> > MACRO;
> > }
> > ```
> Renaming the spelling location seems like a bad idea in this case, I would
> argue we shouldn't rename in macro bodies (see the relevant comment on the
> test-case, let's move discussion there maybe?)
>
> It would also be good to handle rename in macros outside
> `tooling::Replacement`, it's an important decision and I we should not
> blindly stick to the default behavior of `tooling::Replacement` without
> explicitly explaining why it's the best thing for rename.
Yes, I agree. There was a FIXME about this, I was planning to fix this in a
follow-up, but now it is fixed.
================
Comment at: clang-tools-extra/clangd/unittests/RenameTests.cpp:202
+ R"cpp(
+ #define moo [[foo]]
+ int [[fo^o]]() { return 42; }
----------------
ilya-biryukov wrote:
> I would argue we should never rename in macro bodies.
> That might cause problems in other uses of the macro as the token might refer
> to a completely different thing in some other place.
> It's also a pretty rare case, so failing here instead of breaking code seems
> like the proper trade-off.
>
> Are we keen on supporting this? If yes, why?
+1 on your suggestion.. The new code should handle this case as well.
================
Comment at: clang-tools-extra/clangd/unittests/RenameTests.cpp:234
+ template <typename T>
+ class [[F^oo]] {
+ public:
----------------
ilya-biryukov wrote:
> Could we also check partial and full specializations work?
> ```
> template <>
> class Foo<bool> {};
>
> template <class T>
> class Foo<T*> {};
>
> Foo<int> x;
> Foo<bool> y;
> Foo<int*> z;
>
> ```
added.
================
Comment at: clang-tools-extra/clangd/unittests/RenameTests.cpp:313
+ R"cpp(
+ #define NAMESPACE namespace A
+ NAMESPACE {
----------------
ilya-biryukov wrote:
> Why does this have to be a macro? This test does not seem to test macros in
> any way?
>
> Maybe inline the macro?
this was from the original clang-rename test, didn't know the intention about
it.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D69934/new/
https://reviews.llvm.org/D69934
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
