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

Thu Feb 16 14:03:38 PST 2023

DmitryPolukhin added a comment.

@nridge I'm sorry, I pushed version without all tests. Now I added test case for response files, it seems that clangd hasn't had it before this patch.

================
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();
----------------
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.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143436