[PATCH] D71880: [clangd] Implement Decl canonicalization rules for rename

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

don't dig into details, first round of comments.

I think the scope is a bit ambiguous, my reading is that it 1) removes old clang-rename API usage *and* 2) fixes some existing bugs. I would suggest just scoping it on 1).



================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:251
+const NamedDecl *canonicalRenameDecl(const FieldDecl *D) {
+  // This is a hacky way to do something like
+  // CXXMethodDecl::getINstantiatedFromMemberFunction for the field because
----------------
yeah, this is quite tricky. I'd suggest to defer it to a separate patch. 

The behavior you described in https://github.com/clangd/clangd/issues/582 makes sense to me.

Another missing case is about static class members, they are `VarDecl` in the AST. 


================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:283
+    const auto *Definition = RD->getDefinition();
+    Candidate = Definition ? Definition : RD->getMostRecentDecl();
+  }
----------------
I'm not sure the motivation of the change,  the same as line 244.


================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:299
+  if (const auto *Field = dyn_cast<FieldDecl>(Candidate))
+    return canonicalRenameDecl(Field);
+  return Candidate;
----------------
Looks like we're missing the `VarDecl` case (looks like a regression). I think we should probably add tests from https://github.com/llvm/llvm-project/commit/1e32df2f91f1aa1f8cd400ce50a621578fa0534e to clangd rename test.


================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:300
+    return canonicalRenameDecl(Field);
+  return Candidate;
+}
----------------
we're comparing Decl pointers to check whether they point to the same symbol, I think we should use `Candidate->getCanonicalDecl()` here.

thinking of a case, in the AST, we have two `Decl*`s, one points to the forward decl, the other one points to the definition. but they points to the same symbol.

```
class Foo; // forward decl, this is the canonical decl.

class Foo {
};
```



================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:307
   trace::Span Tracer("FindOccurrencesWithinFile");
-  // If the cursor is at the underlying CXXRecordDecl of the
-  // ClassTemplateDecl, ND will be the CXXRecordDecl. In this case, we need to
-  // get the primary template manually.
-  // getUSRsForDeclaration will find other related symbols, e.g. virtual and its
-  // overriddens, primary template and all explicit specializations.
-  // FIXME: Get rid of the remaining tooling APIs.
-  const auto *RenameDecl =
-      ND.getDescribedTemplate() ? ND.getDescribedTemplate() : &ND;
-  std::vector<std::string> RenameUSRs =
-      tooling::getUSRsForDeclaration(RenameDecl, AST.getASTContext());
-  llvm::DenseSet<SymbolID> TargetIDs;
-  for (auto &USR : RenameUSRs)
-    TargetIDs.insert(SymbolID(USR));
+  const auto *RenameDecl = canonicalRenameDecl(&ND);
 
----------------
since we call `canonicalRenameDecl` in `locateDeclAt`, I think `ND` is already canonical, right?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71880