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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
