Page MenuHomePhabricator

[clang][HeaderSuggestion] Handle the case of dotdot with an absolute path

Authored by kadircet on Apr 18 2019, 8:27 AM.



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.

Diff Detail


Event Timeline

kadircet created this revision.Apr 18 2019, 8:27 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 18 2019, 8:27 AM
sammccall accepted this revision.Apr 18 2019, 10:15 AM

Well done!
I never managed to track this one down, this was really annoying.

This revision is now accepted and ready to land.Apr 18 2019, 10:15 AM
ormris added inline comments.Apr 18 2019, 11:41 AM
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.

sammccall added inline comments.Apr 18 2019, 12:57 PM
220 ↗(On Diff #195751)

testPath() returns an appropriate native path, and calculate() returns an #include string (which always uses /).
The "../" on line 217 looks a little more suspicious, but I think the clang driver might silently Do What I Mean here.
Are you seeing test failures?

ormris added inline comments.Apr 18 2019, 1:21 PM
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\""
sammccall added inline comments.Apr 18 2019, 1:55 PM
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.

Sent out D60995, hopefully it should fix the issue. Will wait until that lands.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptApr 24 2019, 2:21 AM