Page MenuHomePhabricator

[clangd] Include insertion: require header guards, drop other heuristics, treat .def like .inc.
ClosedPublic

Authored by sammccall on Apr 5 2019, 7:25 AM.

Details

Summary

We do have some reports of include insertion behaving badly in some
codebases. Requiring header guards both makes sense in principle, and is
likely to disable this "nice-to-have" feature in codebases where headers don't
follow the expected pattern.

With this we can drop some other heuristics, such as looking at file
extensions to detect known non-headers - implementation files have no guards.

One wrinkle here is #import - objc headers may not have guards because
they're intended to be used via #import. If the header is the main file
or is #included, we won't collect locations - merge should take care of
this if we see the file #imported somewhere. Seems likely to be OK.

Headers which have a canonicalization (stdlib, IWYU) are exempt from this check.
*.inc files continue to be handled by looking up to the including file.
This patch also adds *.def here - tablegen wants this pattern too.

In terms of code structure, the division between SymbolCollector and
CanonicalIncludes has shifted: SymbolCollector is responsible for more.
This is because SymbolCollector has all the SourceManager/HeaderSearch access
needed for checking for guards, and we interleave these checks with the *.def
checks in a loop (potentially).
We could hand all the info into CanonicalIncludes and put the logic there
if that's preferable.

Diff Detail

Repository
rL LLVM

Event Timeline

sammccall created this revision.Apr 5 2019, 7:25 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 5 2019, 7:25 AM
sammccall marked an inline comment as done.Apr 5 2019, 7:51 AM
sammccall added inline comments.
unittests/clangd/FileIndexTests.cpp
214 ↗(On Diff #193872)

I suspect we can replace most of these tests with TestTU - here I just changed the ones where the include guard would otherwise be needed.

ioeric added inline comments.Apr 8 2019, 5:31 AM
clangd/index/SymbolCollector.cpp
157 ↗(On Diff #193872)

nit: no need for else?

unittests/clangd/FileIndexTests.cpp
214 ↗(On Diff #193872)

This looks good to me.

I think this test case tried to explicitly test that preamble callback has the expected CanonicalIncludes. Using TestTU seems to achieve the same goal but makes the intention a bit less clear though.

unittests/clangd/TestTU.h
55 ↗(On Diff #193872)

is this used?

ioeric requested changes to this revision.Apr 12 2019, 4:47 AM

in case you missed this patch :)

This revision now requires changes to proceed.Apr 12 2019, 4:47 AM
sammccall updated this revision to Diff 195519.Apr 17 2019, 1:22 AM
sammccall marked 3 inline comments as done.

address review comments

ioeric accepted this revision.Apr 17 2019, 1:49 AM
This revision is now accepted and ready to land.Apr 17 2019, 1:49 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptApr 17 2019, 3:34 AM