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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.


================
Comment at: clang-tools-extra/clangd/unittests/CompilerTests.cpp:59
+
+TEST(BuildCompilerInvocation, MultiArch) {
+  TestTU TU = TestTU::withHeaderCode(R"cpp(
----------------
maybe move this test to `clang/unittests/Frontend/ASTUnitTest.cpp`? someone might send a patch to handle `-arch` specifically in clangd and it would no longer be possible to test this behaviour :)

(another option would be to test this through some other flags that'll trigger multiple compiler invocations, but I don't have any on top of my head)


================
Comment at: clang-tools-extra/clangd/unittests/CompilerTests.cpp:68
+  TU.ExtraArgs = {
+      "--target=x86_64-apple-darwin20", // Needed for driver multi-arch.
+      "-arch", "i386", "-arch", "x86_64"};
----------------
nit: `--target=macho` is enough. and AFAICT root reason to support multiple architectures is mach object type.


================
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);
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107632