kadircet accepted this revision.
kadircet added a comment.
This revision is now accepted and ready to land.
thanks, lgtm!
================
Comment at: clang-tools-extra/clangd/Compiler.cpp:42
const clang::Diagnostic &Info) {
+ DiagnosticConsumer::HandleDiagnostic(DiagLevel, Info);
IgnoreDiagnostics::log(DiagLevel, Info);
----------------
not sure i follow this change. is this for making sure fatal/error diagnostic
counts can be checked (so that actions can signal failure) or something else?
================
Comment at: clang/lib/Frontend/CreateInvocationFromCommandLine.cpp:92
- const driver::Command &Cmd = cast<driver::Command>(*Jobs.begin());
- if (StringRef(Cmd.getCreator().getName()) != "clang") {
Diags->Report(diag::err_fe_expected_clang_command);
----------------
sammccall wrote:
> kadircet wrote:
> > hmm, this looks like a subtle behaviour change. I definitely like the
> > behaviour you proposed better, but maybe do it in a separate patch for
> > making the revert easier if need be?
> Which behavior change are do you mean?
>
> When RecoverFromErrors is false and OffloadCompilation is false, behavior is
> same as before: if there isn't exactly one job we diagnose that and exit, if
> it's not suitable we diagnose that and exit.
>
> When RecoverFromErrors is true we search for the first suitable job, instead
> of complaining that the number is wrong or the one at the front isn't
> suitable, this is the intended change (and basically only affects clangd).
>
> When OffloadCompilation is true, there is a behavior change: previously we'd
> use the first job and fail if it's not clang, now we'll use the first clang
> job. The old behavior seems brittle and the change only affects (previously)
> error cases, and I'd have to add more complexity to preserve it - is this
> worthwhile?
> When OffloadCompilation is true, there is a behavior change: previously we'd
> use the first job and fail if it's not clang, now we'll use the first clang
> job.
I was talking about this particular change.
> The old behavior seems brittle and the change only affects (previously) error
> cases
As mentioned I also like the new behaviour more, I just don't know about what
the offloading logic does (i suppose it is about cuda compilation) and I was
anxious about it breaking when one of the Jobs satisfy the constraint but it is
not the first one.
> and I'd have to add more complexity to preserve it - is this worthwhile?
One option could be to just keep this part the same (i.e. only use the first
Job as long as `PickFirstOfMany` is set, even if `OffloadCompilation` is
false). Then send a follow-up change, that'll search for a suitable job. That
way we can only revert the last one if it breaks and know the constraints. (And
decide to do the search iff `ShouldRecoverOnErorrs && !Offload`). But it is not
that important, feel free to just land as-is.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D107632/new/
https://reviews.llvm.org/D107632
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
