Page MenuHomePhabricator

[clangd] Recognize "don't include me directly" pattern, and suppress include insertion.
ClosedPublic

Authored by sammccall on Apr 17 2019, 3:37 AM.

Details

Summary

Typically used with umbrella headers, e.g. GTK:

#if !defined (GTK_H_INSIDE) && !defined (GTK_COMPILATION)
#error "Only <gtk/gtk.h> can be included directly."
#endif

Heuristic is fairly conservative, a quick code search over github showed
a fair number of hits and few/no false positives. (Not all were umbrella
headers, but I'd be happy avoiding include insertion for all of them).

We may want to relax the heuristic later to catch more cases.

Event Timeline

sammccall created this revision.Apr 17 2019, 3:37 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 17 2019, 3:37 AM
sammccall updated this revision to Diff 195533.Apr 17 2019, 3:39 AM

unconfusing my git repo

sammccall updated this revision to Diff 195534.Apr 17 2019, 3:42 AM
sammccall marked an inline comment as done.

remove leftover debugging

sammccall marked an inline comment as done.Apr 17 2019, 3:43 AM
sammccall added inline comments.
clangd/index/SymbolCollector.cpp
602 ↗(On Diff #195562)

this function has been moved to a member so it can call isSelfContainedHeader which is now a member.

Apart from no longer having to change so many params, it is unmodified.

631 ↗(On Diff #195562)

this has been moved to a member so it can use the cache and the (non-threadsafe) regex, and its logic has obviously been changed.

ioeric accepted this revision.Apr 17 2019, 6:43 AM

lgtm

clangd/index/SymbolCollector.cpp
631 ↗(On Diff #195562)

"self-contained header" doesn't sound accurate anymore. Maybe something like isIncludable?

clangd/index/SymbolCollector.h
125 ↗(On Diff #195562)

DontIncludeMePattern or DontIncludeMeRegex?

This revision is now accepted and ready to land.Apr 17 2019, 6:43 AM
sammccall updated this revision to Diff 195562.Apr 17 2019, 6:59 AM

DontIncludeMePattern

sammccall marked 2 inline comments as done.Apr 17 2019, 7:03 AM
sammccall added inline comments.
clangd/index/SymbolCollector.cpp
631 ↗(On Diff #195562)

Let me explain how I think about it, and then I'll change the name if you still don't think it fits.

What we're detecting here (usually) is that this header can't be included unless a certain symbol/state is defined first from the outside.
Whether that's for some direct technical reason (e.g. configuration is needed) or to enforce a coding style, the dependency on external preprocessor state means the header isn't self contained. (And the non-self-containedness is the reason we can't include it in arbitrary contexts)

ioeric added inline comments.Apr 17 2019, 7:06 AM
clangd/index/SymbolCollector.cpp
631 ↗(On Diff #195562)

That sounds good. Thanks for explaining!

This revision was automatically updated to reflect the committed changes.