sammccall accepted this revision.
sammccall added inline comments.
This revision is now accepted and ready to land.
================
Comment at: clang-tools-extra/clangd/support/Path.h:50
+/// If \p Prefix doesn't match, leaves \p Path untouched and returns false.
+bool pathConsumeFront(PathRef &Path, PathRef Prefix);
+
----------------
This function is a bit of a trap.
c.f. pathStartsWith which understands path semantics: `pathStartsWith("a/b",
"a/bc")` is false.
This function returns true/false based on paths semantics, but the stripping is
lexical:
- `pathConsumeFront("a/b/c", "a/b/")` is "c"
- `pathConsumeFront("a/b/c", "a/b")` is "/c", a very different path.
It's safe where called here because we know testRoot() ends in a slash, but I'd
probably inline it for that reason - it's not as reusable as it looks.
Up to you, though.
================
Comment at: clang-tools-extra/clangd/unittests/HeaderSourceSwitchTests.cpp:277
+#ifdef _WIN32
+// Outside of windows systems, we usually have case sensitive file systems.
+TEST(HeaderSourceSwitchTest, CaseSensitivity) {
----------------
mac is the same as windows here I think.
This should probably be `#ifdef CLANGD_PATH_CASE_INSENSITIVE` if possible?
If you want to be really clever, you could keep the test on all platforms and
just `#ifdef` the assertions to give opposite results. We don't want tests to
pass if we are case insensitive everywhere, right?
================
Comment at: clang-tools-extra/clangd/unittests/HeaderSourceSwitchTests.cpp:281
+ TU.HeaderCode = R"cpp(
+ inline void bar1() {}
+ inline void bar2() {}
----------------
are the inline functions essential to this test?
Even with context I was struggling a bit to understand what this is testing, so
it'd be nice to cut down a few elements
================
Comment at: clang-tools-extra/clangd/unittests/HeaderSourceSwitchTests.cpp:294
+
+ auto HeaderAbsPath = testPath(TU.HeaderFilename);
+ HeaderAbsPath[0] = llvm::toLower(HeaderAbsPath[0]);
----------------
either a comment or a variable name explaining what we're doing here?
// Providing the "wrong"-case drive letter in the query should still find the
file.
Why uppercase the drive letter only and not the whole filename? Then we could
run the test on mac too
================
Comment at: clang-tools-extra/clangd/unittests/HeaderSourceSwitchTests.cpp:295
+ auto HeaderAbsPath = testPath(TU.HeaderFilename);
+ HeaderAbsPath[0] = llvm::toLower(HeaderAbsPath[0]);
+ EXPECT_THAT(testing::StrCaseEq(testPath(TU.Filename)),
----------------
May be nice to have an assertion that HeaderAbsPath !=
testPath(TU.HeaderFilename), so that we're not spuriously passing because
testRoot() is "C:\" or "\\something"
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D121286/new/
https://reviews.llvm.org/D121286
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
