[PATCH] D71598: [clangd] Filter implicit references from index while renaming

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.


================
Comment at: clang-tools-extra/clangd/SourceCode.cpp:217
 
+SourceLocation positionToSourceLoc(const SourceManager &SM, Position Pos,
+                                   StringRef Filename) {
----------------
this one isn't used anywhere?


================
Comment at: clang-tools-extra/clangd/SourceCode.cpp:1136
 
+std::vector<Range> filterRenameRanges(const SourceManager &SM,
+                                      StringRef Filename,
----------------
SM doesn't seem to be necessary, as `lex` already provides that in the callback.


================
Comment at: clang-tools-extra/clangd/SourceCode.h:301
 
+/// Removes ranges with implicit references to the renamed symbol (e.g. in macro
+/// expansions).
----------------
i don't think it is necessary for this function to be made public, it should be OK for it to leave in rename.cpp as a helper.


================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:368
+    // Filter out possible implicit references returned from the index.
+    const auto FilteredRanges = filterRenameRanges(
+        SM, FileAndOccurrences.first(),
----------------
this one should go after `adjustRenameRanges`


================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:368
+    // Filter out possible implicit references returned from the index.
+    const auto FilteredRanges = filterRenameRanges(
+        SM, FileAndOccurrences.first(),
----------------
kadircet wrote:
> this one should go after `adjustRenameRanges`
both this and `adjustRenameRanges` seems to be lexing the source code to get identifier locations.

can we lex the file only a single time instead and make use of the result in both of the functions?
I would suggest moving `collectIdentifierRanges` into here and passing the result as a parameter to both of the functions.

as for implementation of `filterRenameRanges` you might wanna return intersection of `RenameRanges` and result of `collectIdentifierRanges`


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

https://reviews.llvm.org/D71598