Include insertion in clangd was inserting absolute paths when the
include directory was an absolute path with a double dot. This patch makes sure
double dots are handled both with absolute and relative paths.
Details
- Reviewers
sammccall - Commits
- rG936c67d3efa3: [clang][HeaderSuggestion] Handle the case of dotdot with an absolute path
rCTE359078: [clang][HeaderSuggestion] Handle the case of dotdot with an absolute path
rC359078: [clang][HeaderSuggestion] Handle the case of dotdot with an absolute path
rL359078: [clang][HeaderSuggestion] Handle the case of dotdot with an absolute path
Diff Detail
- Repository
- rL LLVM
Event Timeline
clang-tools-extra/unittests/clangd/HeadersTests.cpp | ||
---|---|---|
220 ↗ | (On Diff #195751) | Quick nit: This test won't work on Windows as it only tests for POSIX-style path separators. You could easily add a Windows ifdef, though. |
clang-tools-extra/unittests/clangd/HeadersTests.cpp | ||
---|---|---|
220 ↗ | (On Diff #195751) | testPath() returns an appropriate native path, and calculate() returns an #include string (which always uses /). |
clang-tools-extra/unittests/clangd/HeadersTests.cpp | ||
---|---|---|
220 ↗ | (On Diff #195751) | Yes, here's the output. [ RUN ] HeadersTest.ShortenedInclude C:\llvm\tools\clang\tools\extra\unittests\clangd\HeadersTests.cpp(220): error: Expected: calculate(BarHeader) Which is: "\"sub\\bar.h\"" To be equal to: "\"sub/bar.h\"" |
clang-tools-extra/unittests/clangd/HeadersTests.cpp | ||
---|---|---|
220 ↗ | (On Diff #195751) | Hmm, that suggests the test is catching a real bug in the code: we should be inserting paths with forward slashes even on Windows. Looking at the test cases, i have the sinking feeling we've always been doing this and never noticed as no test case has a slash in the final #include. (And none of the main clangd devs are primarily using Windows) Thank you for pointing this out! @kadircet: we should either fix this before this patch lands, or ifdef it until we can fix. |