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

[Sam McCall via Phabricator via cfe-commits]

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


================
Comment at: clang-tools-extra/clangd/Compiler.cpp:42
                                          const clang::Diagnostic &Info) {
+  DiagnosticConsumer::HandleDiagnostic(DiagLevel, Info);
   IgnoreDiagnostics::log(DiagLevel, Info);
----------------
kadircet wrote:
> 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?
Oops, this was leftover from when we were testing this in clangd. (And was for the reason you suggested, just for testing). Removed.


================
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);
----------------
kadircet wrote:
> 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.
It makes sense. I think i'm happy enough with this patch just getting reverted in such a case though, especially since it's just "defense in depth" currently rather than "load-bearing".


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107632