[PATCH] D121286: [clangd] Handle case insensitive file systems in header/source switch

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet marked 3 inline comments as done.
kadircet added inline comments.


================
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);
+
----------------
sammccall wrote:
> 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.
fair point, moved into TestFS.cpp.


================
Comment at: clang-tools-extra/clangd/unittests/HeaderSourceSwitchTests.cpp:281
+  TU.HeaderCode = R"cpp(
+  inline void bar1() {}
+  inline void bar2() {}
----------------
sammccall wrote:
> 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
yes, we need majority of the symbols to be defined in the header, so that the heuristic picks the header as the implementation file (if the header isn't ignored due to case-sensitive check of filepaths). adding a comment though.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121286