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

Sat Apr 18 08:37:13 PDT 2020

kadircet accepted this revision.
kadircet added a comment.
This revision is now accepted and ready to land.

thanks, lgtm!

================
Comment at: clang/lib/Lex/PPDirectives.cpp:2070
+         diag::err_pp_including_mainfile_in_preamble);
+    return {ImportAction::None};
+  }
----------------
sammccall wrote:
> kadircet wrote:
> > 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?)
> 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...)
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.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D78366/new/

https://reviews.llvm.org/D78366