[PATCH] D69263: [clangd] Implement cross-file rename.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.


================
Comment at: clang-tools-extra/clangd/tool/ClangdMain.cpp:269
+    cat(Features),
+    desc("Enable cross-file rename feature."),
+    init(false),
----------------
Could you document that the feature is highly experimental and may lead to broken code or incomplete renames for the time being?

Also, maybe make it hidden for now? 
At least until we have basic validation of ranges from the index. In the current state we can easily produce edits to unrelated parts of the file...


================
Comment at: clang-tools-extra/clangd/unittests/RenameTests.cpp:277
+  //   - file on VFS for bar.cc;
+  Annotations MainCode("class  [[Fo^o]] {};");
+  auto MainFilePath = testPath("main.cc");
----------------
Thinking whether we can encode these transformations in a nicer way?
If we could convert the contents dirty buffer and replacements to something like:
```
class [[Foo |-> newName]] {};
```
Just a proposal, maybe there's a better syntax here.

We can easily match strings instead of matching ranges in the tests. This has the advantage of having textual diffs in case something goes wrong - much more pleasant than looking at the offsets in the ranges.

WDYT?


================
Comment at: clang-tools-extra/clangd/unittests/RenameTests.cpp:302
+      testing::UnorderedElementsAre(
+          PairMatcher(Eq(FooPath),
+                      EqualFileEdit(FooDirtyBuffer, FooCode.ranges())),
----------------
Instead of defining custom matchers, could we convert to standard types and use existing gtest matchers?
Something like `std::vector<std::pair</*Path*/std::string, Edit>>` should work just fine.

Huge advantage we'll get is better output in case of errors.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69263