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

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.


================
Comment at: clang-tools-extra/clangd/tool/ClangdMain.cpp:269
+    cat(Features),
+    desc("Enable cross-file rename feature."),
+    init(false),
----------------
ilya-biryukov wrote:
> 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...
Done.  yes, we should make it hidden.


================
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");
----------------
ilya-biryukov wrote:
> 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?
I agree textual diff would give a more friendly results when there are failures in the tests.

I don't have a better way to encode the transformation in the annotation code, I think a better way maybe to use a hard-coded new name, and applying the actual replacements on the testing code, and verify the the text diffs.

If we want to do this change, I'd prefer to do it in a followup, since it would change the existing testcases as well. What do you think?



================
Comment at: clang-tools-extra/clangd/unittests/RenameTests.cpp:302
+      testing::UnorderedElementsAre(
+          PairMatcher(Eq(FooPath),
+                      EqualFileEdit(FooDirtyBuffer, FooCode.ranges())),
----------------
ilya-biryukov wrote:
> 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.
yeah, converting the `llvm::StringMap` to standard types could work, but we have to pay the cost of transformation (that was something I tried to avoid).
 I think there is no strong reason to use llvm::StringMap as the `FileEdits`, what do you think if we change the `FileEdits` type to `std::vector<pair<string, Edit>>`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69263