[PATCH] D143436: [clangd] Apply standard adaptors to CDBs pushed from LSP

[Nathan Ridge via Phabricator via cfe-commits]

nridge added inline comments.


================
Comment at: clang-tools-extra/clangd/CompileCommands.cpp:222
+  tooling::addExpandedResponseFiles(Cmd, Command.Directory, Tokenizer, *FS);
+  tooling::addTargetAndModeForProgramName(Cmd, Cmd.front());
   auto &OptTable = clang::driver::getDriverOptTable();
----------------
DmitryPolukhin wrote:
> nridge wrote:
> > nridge wrote:
> > > The target needs to be added **after** the call to `SystemIncludeExtractor` later in this function (this is what D138546 is trying to fix). The reason is that `SystemIncludeExtractor` includes any `--target` flag in the compiler driver being queried for system includes, which may be gcc, which does not support `--target`.
> > (I guess we could make that change separately in D138546, but if we're changing the place where the `--target` is added in this patch, I figure we might as well move it directly to the desired place.)
> I think there are order problems here:
> - we need `--driver-mode=cl` injected here to make check on line #229 work as expected
> - if we don't inject it driver mode here, command line edits won't see the change; see how I modified test clangd/unittests/CompileCommandsTests.cpp line #203, with D138546 edits were not applied properly to driver mode
> 
> I'll double check how it works on Windows but I expect that moving it after SystemIncludeExtractor will break proper driver mode detection.
> I think there are order problems here:
> - we need `--driver-mode=cl` injected here to make check on line #229 work as expected

Looking at the [line in question](https://searchfox.org/llvm/rev/0cbb8ec030e23c0e13331b5d54155def8c901b36/clang-tools-extra/clangd/CompileCommands.cpp#213), it calls `driver::getDriverMode()` which [falls back on extracting the driver mode](https://searchfox.org/llvm/rev/0cbb8ec030e23c0e13331b5d54155def8c901b36/clang/lib/Driver/Driver.cpp#6407) from the program name anyways -- so I think that should be fine.

> - if we don't inject it driver mode here, command line edits won't see the change; see how I modified test clangd/unittests/CompileCommandsTests.cpp line #203, with D138546 edits were not applied properly to driver mode

I'm not following the motivation for this test. Is there any real-world use case which requires the command line edits seeing the `--driver-mode` parameter?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143436