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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov 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
----------------
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.


================
Comment at: clang-tools-extra/clangd/unittests/RenameTests.cpp:202
+      R"cpp(
+        #define moo [[foo]]
+        int [[fo^o]]() { return 42; }
----------------
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?


================
Comment at: clang-tools-extra/clangd/unittests/RenameTests.cpp:234
+        template <typename T>
+        class [[F^oo]] {
+        public:
----------------
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;

```


================
Comment at: clang-tools-extra/clangd/unittests/RenameTests.cpp:313
+      R"cpp(
+        #define NAMESPACE namespace A
+        NAMESPACE {
----------------
Why does this have to be a macro? This test does not seem to test macros in any way?

Maybe inline the macro?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69934