[llvm-branch-commits] [clang-tools-extra] d840463 - [clangd] Treat paths case-insensitively depending on the platform
Tom Stellard via llvm-branch-commits
llvm-branch-commits at lists.llvm.org
Mon Feb 22 12:19:33 PST 2021
Author: Kadir Cetinkaya
Date: 2021-02-22T12:19:07-08:00
New Revision: d8404633401509936600b60274b72fc03f11f040
URL: https://github.com/llvm/llvm-project/commit/d8404633401509936600b60274b72fc03f11f040
DIFF: https://github.com/llvm/llvm-project/commit/d8404633401509936600b60274b72fc03f11f040.diff
LOG: [clangd] Treat paths case-insensitively depending on the platform
Path{Match,Exclude} and MountPoint were checking paths case-sensitively
on all platforms, as with other features, this was causing problems on
windows. Since users can have capital drive letters on config files, but
editors might lower-case them.
This patch addresses that issue by:
- Creating regexes with case-insensitive matching on those platforms.
- Introducing a new pathIsAncestor helper, which performs checks in a
case-correct manner where needed.
Differential Revision: https://reviews.llvm.org/D96690
(cherry picked from commit ecea7218fb9b994b26471e9877851cdb51a5f1d4)
Added:
clang-tools-extra/clangd/unittests/support/PathTests.cpp
Modified:
clang-tools-extra/clangd/ConfigCompile.cpp
clang-tools-extra/clangd/support/Path.cpp
clang-tools-extra/clangd/support/Path.h
clang-tools-extra/clangd/unittests/CMakeLists.txt
clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp
clang-tools-extra/clangd/unittests/RenameTests.cpp
Removed:
################################################################################
diff --git a/clang-tools-extra/clangd/ConfigCompile.cpp b/clang-tools-extra/clangd/ConfigCompile.cpp
index 8682cae36f26..dadc578c3a81 100644
--- a/clang-tools-extra/clangd/ConfigCompile.cpp
+++ b/clang-tools-extra/clangd/ConfigCompile.cpp
@@ -31,6 +31,7 @@
#include "Features.inc"
#include "TidyProvider.h"
#include "support/Logger.h"
+#include "support/Path.h"
#include "support/Trace.h"
#include "llvm/ADT/None.h"
#include "llvm/ADT/Optional.h"
@@ -101,9 +102,11 @@ struct FragmentCompiler {
// Normalized Fragment::SourceInfo::Directory.
std::string FragmentDirectory;
- llvm::Optional<llvm::Regex> compileRegex(const Located<std::string> &Text) {
+ llvm::Optional<llvm::Regex>
+ compileRegex(const Located<std::string> &Text,
+ llvm::Regex::RegexFlags Flags = llvm::Regex::NoFlags) {
std::string Anchored = "^(" + *Text + ")$";
- llvm::Regex Result(Anchored);
+ llvm::Regex Result(Anchored, Flags);
std::string RegexError;
if (!Result.isValid(RegexError)) {
diag(Error, "Invalid regex " + Anchored + ": " + RegexError, Text.Range);
@@ -195,9 +198,15 @@ struct FragmentCompiler {
if (F.HasUnrecognizedCondition)
Out.Conditions.push_back([&](const Params &) { return false; });
+#ifdef CLANGD_PATH_CASE_INSENSITIVE
+ llvm::Regex::RegexFlags Flags = llvm::Regex::IgnoreCase;
+#else
+ llvm::Regex::RegexFlags Flags = llvm::Regex::NoFlags;
+#endif
+
auto PathMatch = std::make_unique<std::vector<llvm::Regex>>();
for (auto &Entry : F.PathMatch) {
- if (auto RE = compileRegex(Entry))
+ if (auto RE = compileRegex(Entry, Flags))
PathMatch->push_back(std::move(*RE));
}
if (!PathMatch->empty()) {
@@ -218,7 +227,7 @@ struct FragmentCompiler {
auto PathExclude = std::make_unique<std::vector<llvm::Regex>>();
for (auto &Entry : F.PathExclude) {
- if (auto RE = compileRegex(Entry))
+ if (auto RE = compileRegex(Entry, Flags))
PathExclude->push_back(std::move(*RE));
}
if (!PathExclude->empty()) {
@@ -349,7 +358,8 @@ struct FragmentCompiler {
return;
Spec.MountPoint = std::move(*AbsPath);
Out.Apply.push_back([Spec(std::move(Spec))](const Params &P, Config &C) {
- if (!P.Path.startswith(Spec.MountPoint))
+ if (P.Path.empty() || !pathStartsWith(Spec.MountPoint, P.Path,
+ llvm::sys::path::Style::posix))
return;
C.Index.External = Spec;
// Disable background indexing for the files under the mountpoint.
diff --git a/clang-tools-extra/clangd/support/Path.cpp b/clang-tools-extra/clangd/support/Path.cpp
index f72d00070f34..6fc74b92fc7a 100644
--- a/clang-tools-extra/clangd/support/Path.cpp
+++ b/clang-tools-extra/clangd/support/Path.cpp
@@ -7,24 +7,33 @@
//===----------------------------------------------------------------------===//
#include "support/Path.h"
+#include "llvm/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
-}
+#ifdef CLANGD_PATH_CASE_INSENSITIVE
+std::string maybeCaseFoldPath(PathRef Path) { return Path.lower(); }
+bool pathEqual(PathRef A, PathRef B) { return A.equals_lower(B); }
+#else // NOT CLANGD_PATH_CASE_INSENSITIVE
+std::string maybeCaseFoldPath(PathRef Path) { return Path.str(); }
+bool pathEqual(PathRef A, PathRef B) { return A == B; }
+#endif // CLANGD_PATH_CASE_INSENSITIVE
-bool pathEqual(PathRef A, PathRef B) {
-#if defined(_WIN32) || defined(__APPLE__)
- return A.equals_lower(B);
-#else
- return A == B;
-#endif
+bool pathStartsWith(PathRef Ancestor, PathRef Path,
+ llvm::sys::path::Style Style) {
+ assert(llvm::sys::path::is_absolute(Ancestor, Style) &&
+ llvm::sys::path::is_absolute(Path, Style));
+ // If ancestor ends with a separator drop that, so that we can match /foo/ as
+ // a parent of /foo.
+ if (llvm::sys::path::is_separator(Ancestor.back(), Style))
+ Ancestor = Ancestor.drop_back();
+ // Ensure Path starts with Ancestor.
+ if (!pathEqual(Ancestor, Path.take_front(Ancestor.size())))
+ return false;
+ Path = Path.drop_front(Ancestor.size());
+ // Then make sure either two paths are equal or Path has a separator
+ // afterwards.
+ return Path.empty() || llvm::sys::path::is_separator(Path.front(), Style);
}
-
} // namespace clangd
} // namespace clang
diff --git a/clang-tools-extra/clangd/support/Path.h b/clang-tools-extra/clangd/support/Path.h
index 402903130f01..938d7d7e99c9 100644
--- a/clang-tools-extra/clangd/support/Path.h
+++ b/clang-tools-extra/clangd/support/Path.h
@@ -10,8 +10,14 @@
#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_SUPPORT_PATH_H
#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/Path.h"
#include <string>
+/// Whether current platform treats paths case insensitively.
+#if defined(_WIN32) || defined(__APPLE__)
+#define CLANGD_PATH_CASE_INSENSITIVE
+#endif
+
namespace clang {
namespace clangd {
@@ -28,6 +34,12 @@ using PathRef = llvm::StringRef;
std::string maybeCaseFoldPath(PathRef Path);
bool pathEqual(PathRef, PathRef);
+/// Checks if \p Ancestor is a proper ancestor of \p Path. This is just a
+/// smarter lexical prefix match, e.g: foo/bar/baz doesn't start with foo/./bar.
+/// Both \p Ancestor and \p Path must be absolute.
+bool pathStartsWith(
+ PathRef Ancestor, PathRef Path,
+ llvm::sys::path::Style Style = llvm::sys::path::Style::native);
} // namespace clangd
} // namespace clang
diff --git a/clang-tools-extra/clangd/unittests/CMakeLists.txt b/clang-tools-extra/clangd/unittests/CMakeLists.txt
index f4d364720eaf..c396c6f5873b 100644
--- a/clang-tools-extra/clangd/unittests/CMakeLists.txt
+++ b/clang-tools-extra/clangd/unittests/CMakeLists.txt
@@ -104,6 +104,7 @@ add_unittest(ClangdUnitTests ClangdTests
support/FunctionTests.cpp
support/MarkupTests.cpp
support/MemoryTreeTests.cpp
+ support/PathTests.cpp
support/ThreadingTests.cpp
support/TestTracer.cpp
support/TraceTests.cpp
diff --git a/clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp b/clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp
index 4b1da2035727..d9aa171f3102 100644
--- a/clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp
+++ b/clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp
@@ -99,6 +99,25 @@ TEST_F(ConfigCompileTests, Condition) {
Frag.If.PathMatch.emplace_back("ba*r");
EXPECT_FALSE(compileAndApply());
EXPECT_THAT(Diags.Diagnostics, IsEmpty());
+
+ // Only matches case-insensitively.
+ Frag = {};
+ Frag.If.PathMatch.emplace_back("B.*R");
+ EXPECT_THAT(Diags.Diagnostics, IsEmpty());
+#ifdef CLANGD_PATH_CASE_INSENSITIVE
+ EXPECT_TRUE(compileAndApply());
+#else
+ EXPECT_FALSE(compileAndApply());
+#endif
+
+ Frag = {};
+ Frag.If.PathExclude.emplace_back("B.*R");
+ EXPECT_THAT(Diags.Diagnostics, IsEmpty());
+#ifdef CLANGD_PATH_CASE_INSENSITIVE
+ EXPECT_FALSE(compileAndApply());
+#else
+ EXPECT_TRUE(compileAndApply());
+#endif
}
TEST_F(ConfigCompileTests, CompileCommands) {
@@ -406,6 +425,23 @@ TEST_F(ConfigCompileTests, ExternalBlockMountPoint) {
ASSERT_THAT(Diags.Diagnostics, IsEmpty());
ASSERT_TRUE(Conf.Index.External);
EXPECT_THAT(Conf.Index.External->MountPoint, FooPath);
+
+ // Only matches case-insensitively.
+ BazPath = testPath("fOo/baz.h", llvm::sys::path::Style::posix);
+ BazPath = llvm::sys::path::convert_to_slash(BazPath);
+ Parm.Path = BazPath;
+
+ FooPath = testPath("FOO/", llvm::sys::path::Style::posix);
+ FooPath = llvm::sys::path::convert_to_slash(FooPath);
+ Frag = GetFrag("", FooPath.c_str());
+ compileAndApply();
+ ASSERT_THAT(Diags.Diagnostics, IsEmpty());
+#ifdef CLANGD_PATH_CASE_INSENSITIVE
+ ASSERT_TRUE(Conf.Index.External);
+ EXPECT_THAT(Conf.Index.External->MountPoint, FooPath);
+#else
+ ASSERT_FALSE(Conf.Index.External);
+#endif
}
} // namespace
} // namespace config
diff --git a/clang-tools-extra/clangd/unittests/RenameTests.cpp b/clang-tools-extra/clangd/unittests/RenameTests.cpp
index e25850a68fe9..b2c83a1a4303 100644
--- a/clang-tools-extra/clangd/unittests/RenameTests.cpp
+++ b/clang-tools-extra/clangd/unittests/RenameTests.cpp
@@ -1102,7 +1102,7 @@ TEST(RenameTest, IndexMergeMainFile) {
EXPECT_THAT(Results.GlobalChanges.keys(),
UnorderedElementsAre(Main, testPath("other.cc")));
-#if defined(_WIN32) || defined(__APPLE__)
+#ifdef CLANGD_PATH_CASE_INSENSITIVE
// On case-insensitive systems, no duplicates if AST vs index case
diff ers.
// https://github.com/clangd/clangd/issues/665
TU.Filename = "MAIN.CC";
diff --git a/clang-tools-extra/clangd/unittests/support/PathTests.cpp b/clang-tools-extra/clangd/unittests/support/PathTests.cpp
new file mode 100644
index 000000000000..26b999d103a0
--- /dev/null
+++ b/clang-tools-extra/clangd/unittests/support/PathTests.cpp
@@ -0,0 +1,36 @@
+//===-- PathTests.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"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace clangd {
+namespace {
+TEST(PathTests, IsAncestor) {
+ EXPECT_TRUE(pathStartsWith("/foo", "/foo"));
+ EXPECT_TRUE(pathStartsWith("/foo/", "/foo"));
+
+ EXPECT_FALSE(pathStartsWith("/foo", "/fooz"));
+ EXPECT_FALSE(pathStartsWith("/foo/", "/fooz"));
+
+ EXPECT_TRUE(pathStartsWith("/foo", "/foo/bar"));
+ EXPECT_TRUE(pathStartsWith("/foo/", "/foo/bar"));
+
+#ifdef CLANGD_PATH_CASE_INSENSITIVE
+ EXPECT_TRUE(pathStartsWith("/fOo", "/foo/bar"));
+ EXPECT_TRUE(pathStartsWith("/foo", "/fOo/bar"));
+#else
+ EXPECT_FALSE(pathStartsWith("/fOo", "/foo/bar"));
+ EXPECT_FALSE(pathStartsWith("/foo", "/fOo/bar"));
+#endif
+}
+} // namespace
+} // namespace clangd
+} // namespace clang
More information about the llvm-branch-commits
mailing list