sammccall created this revision.
sammccall added a reviewer: hokein.
Herald added subscribers: usaxena95, kadircet, arphaman, mgorny.
sammccall requested review of this revision.
Herald added subscribers: cfe-commits, MaskRay, ilya-biryukov.
Herald added a project: clang.
If you have c:\foo open, and C:\foo indexed (case difference) then these
need to be considered the same file. Otherwise we emit edits to both,
and editors do... something that isn't pretty.
Maybe more centralized normalization is called for, but it's not trivial
to do this while also being case-preserving. see
https://github.com/clangd/clangd/issues/108
Fixes https://github.com/clangd/clangd/issues/665
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D95759
Files:
clang-tools-extra/clangd/GlobalCompilationDatabase.cpp
clang-tools-extra/clangd/refactor/Rename.cpp
clang-tools-extra/clangd/support/CMakeLists.txt
clang-tools-extra/clangd/support/Path.cpp
clang-tools-extra/clangd/support/Path.h
clang-tools-extra/clangd/unittests/RenameTests.cpp
Index: clang-tools-extra/clangd/unittests/RenameTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/RenameTests.cpp
+++ clang-tools-extra/clangd/unittests/RenameTests.cpp
@@ -1067,6 +1067,52 @@
}
}
+MATCHER_P(newText, T, "") { return arg.newText == T; }
+
+TEST(RenameTest, IndexMergeMainFile) {
+ Annotations Code("int ^x();");
+ TestTU TU = TestTU::withCode(Code.code());
+ TU.Filename = "main.cc";
+ auto AST = TU.build();
+
+ auto Main = testPath("main.cc");
+
+ auto Rename = [&](const SymbolIndex *Idx) {
+ auto GetDirtyBuffer = [&](PathRef Path) -> llvm::Optional<std::string> {
+ return Code.code().str(); // Every file has the same content.
+ };
+ RenameOptions Opts;
+ Opts.AllowCrossFile = true;
+ RenameInputs Inputs{Code.point(), "xPrime", AST, Main,
+ Idx, Opts, GetDirtyBuffer};
+ auto Results = rename(Inputs);
+ EXPECT_TRUE(bool(Results)) << llvm::toString(Results.takeError());
+ return std::move(*Results);
+ };
+
+ // We do not expect to see duplicated edits from AST vs index.
+ auto Results = Rename(TU.index().get());
+ EXPECT_THAT(Results.GlobalChanges.keys(), ElementsAre(Main));
+ EXPECT_THAT(Results.GlobalChanges[Main].asTextEdits(),
+ ElementsAre(newText("xPrime")));
+
+ // Sanity check: we do expect to see index results!
+ TU.Filename = "other.cc";
+ Results = Rename(TU.index().get());
+ EXPECT_THAT(Results.GlobalChanges.keys(),
+ UnorderedElementsAre(Main, testPath("other.cc")));
+
+#if defined(_WIN32) || defined(__APPLE__)
+ // On case-insensitive systems, no duplicates if AST vs index case differs.
+ // https://github.com/clangd/clangd/issues/665
+ TU.Filename = "MAIN.CC";
+ Results = Rename(TU.index().get());
+ EXPECT_THAT(Results.GlobalChanges.keys(), ElementsAre(Main));
+ EXPECT_THAT(Results.GlobalChanges[Main].asTextEdits(),
+ ElementsAre(newText("xPrime")));
+#endif
+}
+
TEST(RenameTest, MainFileReferencesOnly) {
// filter out references not from main file.
llvm::StringRef Test =
@@ -1576,43 +1622,43 @@
llvm::StringRef IndexedCode;
llvm::StringRef DraftCode;
} Tests[] = {
- {
- // both line and column are changed, not a near miss.
- R"cpp(
+ {
+ // both line and column are changed, not a near miss.
+ R"cpp(
int [[x]] = 0;
)cpp",
- R"cpp(
+ R"cpp(
// insert a line.
double x = 0;
)cpp",
- },
- {
- // subset.
- R"cpp(
+ },
+ {
+ // subset.
+ R"cpp(
int [[x]] = 0;
)cpp",
- R"cpp(
+ R"cpp(
int [[x]] = 0;
{int x = 0; }
)cpp",
- },
- {
- // shift columns.
- R"cpp(int [[x]] = 0; void foo(int x);)cpp",
- R"cpp(double [[x]] = 0; void foo(double x);)cpp",
- },
- {
- // shift lines.
- R"cpp(
+ },
+ {
+ // shift columns.
+ R"cpp(int [[x]] = 0; void foo(int x);)cpp",
+ R"cpp(double [[x]] = 0; void foo(double x);)cpp",
+ },
+ {
+ // shift lines.
+ R"cpp(
int [[x]] = 0;
void foo(int x);
)cpp",
- R"cpp(
+ R"cpp(
// insert a line.
int [[x]] = 0;
void foo(int x);
)cpp",
- },
+ },
};
LangOptions LangOpts;
LangOpts.CPlusPlus = true;
@@ -1635,69 +1681,62 @@
struct {
llvm::StringRef IndexedCode;
llvm::StringRef LexedCode;
- } Tests[] = {
- {
- // no lexed ranges.
- "[[]]",
- "",
- },
- {
- // both line and column are changed, not a near miss.
- R"([[]])",
- R"(
+ } Tests[] = {{
+ // no lexed ranges.
+ "[[]]",
+ "",
+ },
+ {
+ // both line and column are changed, not a near miss.
+ R"([[]])",
+ R"(
[[]]
)",
- },
- {
- // subset.
- "[[]]",
- "^[[]] [[]]"
- },
- {
- // shift columns.
- "[[]] [[]]",
- " ^[[]] ^[[]] [[]]"
- },
- {
- R"(
+ },
+ {// subset.
+ "[[]]", "^[[]] [[]]"},
+ {// shift columns.
+ "[[]] [[]]", " ^[[]] ^[[]] [[]]"},
+ {
+ R"(
[[]]
[[]] [[]]
)",
- R"(
+ R"(
// insert a line
^[[]]
^[[]] ^[[]]
)",
- },
- {
- R"(
+ },
+ {
+ R"(
[[]]
[[]] [[]]
)",
- R"(
+ R"(
// insert a line
^[[]]
^[[]] ^[[]] // column is shifted.
)",
- },
- {
- R"(
+ },
+ {
+ R"(
[[]]
[[]] [[]]
)",
- R"(
+ R"(
// insert a line
[[]]
[[]] [[]] // not mapped (both line and column are changed).
)",
- },
- {
- R"(
+ },
+ {
+ R"(
[[]]
[[]]
@@ -1706,7 +1745,7 @@
}
)",
- R"(
+ R"(
// insert a new line
^[[]]
^[[]]
@@ -1715,22 +1754,21 @@
^[[]]
[[]] // additional range
)",
- },
- {
- // non-distinct result (two best results), not a near miss
- R"(
+ },
+ {
+ // non-distinct result (two best results), not a near miss
+ R"(
[[]]
[[]]
[[]]
)",
- R"(
+ R"(
[[]]
[[]]
[[]]
[[]]
)",
- }
- };
+ }};
for (const auto &T : Tests) {
SCOPED_TRACE(T.IndexedCode);
auto Lexed = Annotations(T.LexedCode);
Index: clang-tools-extra/clangd/support/Path.h
===================================================================
--- clang-tools-extra/clangd/support/Path.h
+++ clang-tools-extra/clangd/support/Path.h
@@ -22,6 +22,12 @@
/// signatures.
using PathRef = llvm::StringRef;
+// For platforms where paths are case-insensitive (but case-preserving),
+// we need to do case-insensitive comparisons and use lowercase keys.
+// FIXME: Make Path a real class with desired semantics instead.
+std::string maybeCaseFoldPath(PathRef Path);
+bool pathEqual(PathRef, PathRef);
+
} // namespace clangd
} // namespace clang
Index: clang-tools-extra/clangd/support/Path.cpp
===================================================================
--- /dev/null
+++ clang-tools-extra/clangd/support/Path.cpp
@@ -0,0 +1,30 @@
+//===--- Path.cpp -------------------------------------------*- C++-*------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "support/Path.h"
+namespace clang {
+namespace clangd {
+
+std::string maybeCaseFoldPath(PathRef Path) {
+#if defined(_WIN32) || defined(__APPLE__)
+ return Path.lower();
+#else
+ return std::string(Path);
+#endif
+}
+
+bool pathEqual(PathRef A, PathRef B) {
+#if defined(_WIN32) || defined(__APPLE__)
+ return A.equals_lower(B);
+#else
+ return A == B;
+#endif
+}
+
+} // namespace clangd
+} // namespace clang
Index: clang-tools-extra/clangd/support/CMakeLists.txt
===================================================================
--- clang-tools-extra/clangd/support/CMakeLists.txt
+++ clang-tools-extra/clangd/support/CMakeLists.txt
@@ -23,6 +23,7 @@
Logger.cpp
Markup.cpp
MemoryTree.cpp
+ Path.cpp
Shutdown.cpp
Threading.cpp
ThreadsafeFS.cpp
Index: clang-tools-extra/clangd/refactor/Rename.cpp
===================================================================
--- clang-tools-extra/clangd/refactor/Rename.cpp
+++ clang-tools-extra/clangd/refactor/Rename.cpp
@@ -68,7 +68,7 @@
if (OtherFile)
return;
if (auto RefFilePath = filePath(R.Location, /*HintFilePath=*/MainFile)) {
- if (*RefFilePath != MainFile)
+ if (!pathEqual(*RefFilePath, MainFile))
OtherFile = *RefFilePath;
}
});
@@ -468,13 +468,15 @@
// Absolute file path => rename occurrences in that file.
llvm::StringMap<std::vector<Range>> AffectedFiles;
+ llvm::errs() << "querying index\n";
bool HasMore = Index.refs(RQuest, [&](const Ref &R) {
+ llvm::errs() << "found...\n";
if (AffectedFiles.size() >= MaxLimitFiles)
return;
if ((R.Kind & RefKind::Spelled) == RefKind::Unknown)
return;
if (auto RefFilePath = filePath(R.Location, /*HintFilePath=*/MainFile)) {
- if (*RefFilePath != MainFile)
+ if (!pathEqual(*RefFilePath, MainFile))
AffectedFiles[*RefFilePath].push_back(toRange(R.Location));
}
});
@@ -675,6 +677,8 @@
if (!Opts.AllowCrossFile || RenameDecl.getParentFunctionOrMethod()) {
Result.GlobalChanges = FileEdits(
{std::make_pair(RInputs.MainFilePath, std::move(MainFileEdits))});
+ llvm::errs() << "bail\n";
+ llvm::errs() << Opts.AllowCrossFile << "\n";
return Result;
}
Index: clang-tools-extra/clangd/GlobalCompilationDatabase.cpp
===================================================================
--- clang-tools-extra/clangd/GlobalCompilationDatabase.cpp
+++ clang-tools-extra/clangd/GlobalCompilationDatabase.cpp
@@ -395,20 +395,6 @@
return None;
}
-// For platforms where paths are case-insensitive (but case-preserving),
-// we need to do case-insensitive comparisons and use lowercase keys.
-// FIXME: Make Path a real class with desired semantics instead.
-// This class is not the only place this problem exists.
-// FIXME: Mac filesystems default to case-insensitive, but may be sensitive.
-
-static std::string maybeCaseFoldPath(PathRef Path) {
-#if defined(_WIN32) || defined(__APPLE__)
- return Path.lower();
-#else
- return std::string(Path);
-#endif
-}
-
std::vector<DirectoryBasedGlobalCompilationDatabase::DirectoryCache *>
DirectoryBasedGlobalCompilationDatabase::getDirectoryCaches(
llvm::ArrayRef<llvm::StringRef> Dirs) const {
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
