Page MenuHomePhabricator

[clangd] Check file path of declaring header when deciding whether to insert include.
ClosedPublic

Authored by ioeric on Apr 15 2019, 4:41 AM.

Details

Summary

Previously, we would use include spelling of the declaring header to check
whether the inserted header is the same as the main file. This doesn't help because
we only have file path of the main file.

Diff Detail

Repository
rL LLVM

Event Timeline

ioeric created this revision.Apr 15 2019, 4:41 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 15 2019, 4:41 AM
sammccall accepted this revision.Apr 16 2019, 1:51 AM

Thanks!

IIUC, this already fixes the cases we'd seen of include-insertion of a header into itself.
Is it feasible to add a test case for that?

clangd/Headers.h
140 ↗(On Diff #195136)

is the path

This revision is now accepted and ready to land.Apr 16 2019, 1:51 AM
ioeric marked an inline comment as done.Apr 16 2019, 1:57 AM
ioeric added inline comments.
unittests/clangd/HeadersTests.cpp
208 ↗(On Diff #195136)

IIUC, this already fixes the cases we'd seen of include-insertion of a header into itself.
Is it feasible to add a test case for that?

Here is a test case for it. However, to trigger the old behavior caused by include spelling, we would need to implement a custom URI scheme that performs include spelling customization. Let me know if you think we should add one.

sammccall added inline comments.Apr 16 2019, 2:09 AM
unittests/clangd/HeadersTests.cpp
208 ↗(On Diff #195136)

Ah right, this probably isn't worth it :-(

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