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

Sat Apr 18 08:05:04 PDT 2020

sammccall marked an inline comment as done.
sammccall added inline comments.

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

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78366