[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/ClangdServer.cpp:326
+    // class Foo, but the token under the cursor is not corresponding to the
+    // "Foo" range, though the final result is correct.
     SourceLocation Loc = getBeginningOfIdentifier(
----------------
ilya-biryukov wrote:
> hokein wrote:
> > ilya-biryukov wrote:
> > > I would argue rename should not work in that case.
> > > Could we check that the cursor is actually on the name token of the `NamedDecl` and not rename if it isn't?
> > you are probably right, we only allow rename which is triggered on the name token. Will update the patch.
> Definitely think we should do it before landing the patch.
> Otherwise we'll introduce regressions.
Done. 


================
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:
> 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;
}
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69934