[PATCH] D107632: [clangd] Avoid "expected one compiler job" by picking the first eligible job.

[Kadir Cetinkaya via Phabricator via cfe-commits]

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