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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
