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

Authored by kadircet on Apr 18 2019, 8:27 AM.
Tags
• Restricted Project
• Restricted Project
Subscribers

# Details

Summary

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

Repository
rL LLVM

### Event Timeline

kadircet created this revision.Apr 18 2019, 8:27 AM
Herald added a project: Restricted Project. Apr 18 2019, 8:27 AM
Herald added subscribers: cfe-commits, arphaman, jkorous and 2 others.
Herald added a subscriber: ormris. Apr 18 2019, 8:29 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
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.

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?

220 ↗(On Diff #195751)

Yes, here's the output.

[ RUN      ] HeadersTest.ShortenedInclude
Which is: "\"sub\\bar.h\""
To be equal to: "\"sub/bar.h\""
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. Apr 24 2019, 2:21 AM