[PATCH] D71247: [clangd] Rename constructors and destructors in cross-file case

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:92
+                  DeclRelation::Alias | DeclRelation::TemplatePattern)) {
+    const auto *ND = llvm::cast<NamedDecl>(D);
+    // Get to CXXRecordDecl from constructor or destructor.
----------------
OK, this can definitely fail though - just select 'friend X' in a class body or so. Need to check for ND and bail out.


================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:223
-      ND.getDescribedTemplate() ? *ND.getDescribedTemplate() : ND;
-  // getUSRsForDeclaration will find other related symbols, e.g. virtual and its
-  // overriddens, primary template and all explicit specializations.
----------------
you removed this comment and the fixme, but it still applies?


================
Comment at: clang-tools-extra/clangd/unittests/RenameTests.cpp:732
+      {
+        // Constructor.
+      R"cpp(
----------------
nit: put these after "class methods" rather than at the top?


================
Comment at: clang-tools-extra/clangd/unittests/RenameTests.cpp:750
+    {
+      // Destructor (selecting before the identifier).
+      R"cpp(
----------------
4 is too many cases for constructor/destructor.

What about 2: Constructor at start, destructor at end?
Renaming in the middle of the token is covered by existing cases.


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

https://reviews.llvm.org/D71247