[PATCH] D69934: [clangd] Implement rename by using SelectionTree and findExplicitReferences.

[Haojian Wu via Phabricator via cfe-commits]

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