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