[PATCH] D72071: [clangd] Add correctness checks for index-based rename

[Sam McCall via Phabricator via cfe-commits]

sammccall requested changes to this revision.
sammccall added a comment.
This revision now requires changes to proceed.

Postfiltering non-textual references won't work for the index-based rename, as the whole adjust-the-ranges algorithm relies on the index references being a subset of textual matches. More details in https://github.com/clangd/clangd/issues/238

It looks like this code change only affects index-based rename, but the attached test is only for AST-based rename.

I thought the plan of record was to make sure the index knew whether the reference was textual or not, and filter them out before calling adjustRenameRanges?



================
Comment at: clang-tools-extra/clangd/unittests/RenameTests.cpp:798
           [[Foo]] foo;
+          foo.~[[Foo]];
         }
----------------
this isn't valid c++


================
Comment at: clang-tools-extra/clangd/unittests/RenameTests.cpp:882
+          [[Foo]] foo;
+          FooFoo z;
+        }
----------------
If you want to be sure of fixing this bug, I'd like to see a test case: 
 - where the index is out of date
 - where the macro name is unrelated to the symbol name
 - where the macro usages are interleaved with the non-macro usages
 - where the macro usages are in the file which is renamed based on the index, not the one where renamed based on ast


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72071