[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:91
+                  DeclRelation::Alias | DeclRelation::TemplatePattern)) {
+    // If the cursor is at the underlying CXXRecordDecl of the
+    // ClassTemplateDecl, ND will be the CXXRecordDecl. In this case, we need to
----------------
why is this code being moved?


================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:453
+  if (!Offset) {
+    return makeError(ReasonToReject::UsedOutsideFile);
+  }
----------------
so getting an invalid location from the editor isn't the same thing as the symbol being used outside the file! Here we already have an Error to return.
```
if (!Offset)
  return Offset.takeError();
```
but really there's a helper to do this:
```
SourceLocation Loc = sourceLocationInMainFile(SM, RInputs.Pos);
if (!Loc)
  return Loc.takeError();
const syntax::Token *IdentifierToken = spelledIdentifierTouching(Loc, AST.getTokens());
```


================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:456
+  SourceLocation TokenLocation = SM.getComposedLoc(FID, *Offset);
+  const auto IdentifierToken =
+      syntax::spelledIdentifierTouching(TokenLocation, AST.getTokens());
----------------
nit: auto* for pointers, but spelling out the type here would be clearer I think


================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:458
+      syntax::spelledIdentifierTouching(TokenLocation, AST.getTokens());
+  // There should be exactly one identifier token the cursor is pointing to.
+  // Renames should only triggered on identifiers.
----------------
this is an invariant of the function we're calling, not this one - no need to document it (next line is good though)


================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:462
+    return makeError(ReasonToReject::NoSymbolFound);
+  SourceLocation SourceLocationBeg =
+      SM.getMacroArgExpandedLocation(IdentifierToken->location());
----------------
This unwrapping doesn't seem necessary, - it doesn't make a difference for macros (which we don't support anyway), nor for selectiontree. Just inline usages of IdentifierToken->location() below?


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

https://reviews.llvm.org/D71247