[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
   if (const auto *D = SelectedNode->ASTNode.get<Decl>()) {
-    if (D->getLocation() != TokenStartLoc)
-      return {};
+    if (D->getLocation() != TokenStartLoc) {
+      // Destructor->getLocation() points to ~. In this case, TokenStartLoc
----------------
Braindump from an offline conversation...
This seems like an unfortunate place to put a complicated special case and it doesn't generalize well.

The TokenStartLoc check is a bit unfortunate to begin with - it's subject to all the heuristic-token-rewinding problems we've encountered in the past. Its one advantage over plain selectiontree is that it works at the end of the identifier.

Better approach seems to be:
 - we only allow rename targeted at an identifier token
 - there can be at most one identifier touching the location (if there's one before and after, they'd be a single identifier)
 - so find that identifier spelled token using TokenBuffer
 - then look it up as a macro (we get its start location for  free)
 - if that fails, create a selectiontree for the token's range and rename what we find

I don't think there's a better TokenBuffer API than calling spelledTokens() and binary-searching, at the moment.

This touches more code, but is less special-case-y overall. I'd consider putting the getIdentifierTouching(SourceLocation, TokenBuffer, SourceManager) in SourceCode.h


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

https://reviews.llvm.org/D71247