This is an archive of the discontinued LLVM Phabricator instance.

[Preamble] Allow recursive inclusion of header-guarded mainfile.

Authored by sammccall on Apr 17 2020, 7:13 AM.



This is guaranteed to be a no-op without the preamble, so should be a
no-op with it too.

Partially fixes
This doesn't yet work for #ifndef guards, which are not recognized in preambles.
see D78038

I can't for the life of me work out how to test this outside clangd.
The original reentrant preamble diagnostic was untested, I added a test
to clangd for that too.

Diff Detail

Event Timeline

sammccall created this revision.Apr 17 2020, 7:13 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 17 2020, 7:13 AM
kadircet added inline comments.Apr 18 2020, 2:07 AM

Instead of returning here I think we should set the Action to Skip. So that relevant callbacks (InclusionDirective and FileSkipped) is invoked. (maybe have a test for that too?)

sammccall marked an inline comment as done.Apr 18 2020, 7:54 AM
sammccall added inline comments.

FileSkipped says: Callback invoked whenever a source file is skipped as the result of header guard optimization. that's not exactly what's happening here (if it were, Action is set to Skip above and we never enter this if statement).

As for calling InclusionDirective - it looks like some error paths call it and some error paths don't (returning before vs after InclusionDirective(). "Erasing" the illegal include seem valid, as would be skipping it. I'm not sure I want to change the behavior in this patch (and maybe introduce a new bug while fixing this one).

Is there some concrete reason we need this callback?

(As mentioned, I've got no idea how to reasonably test this in a fine-grained way without boiling the ocean, it was never previously tested...)

kadircet accepted this revision.Apr 18 2020, 8:22 AM

thanks, lgtm!


That's how most of the tools collect includes, there are some clang-tidy checkers and also clangd features depend on receiving that callback. In the absence of that the include will be totally invisible to clangd. I suppose its OK to not support goto/documetlink for mainfile itself, it might cause troubles in feature if we plan to make use of a preamble built for one file in other files, but we can worry about that later on.

So it is ok if you want to keep current behaviour.

This revision is now accepted and ready to land.Apr 18 2020, 8:22 AM
This revision was automatically updated to reflect the committed changes.