DmitryPolukhin 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(); ---------------- nridge wrote: > 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? > 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? @nridge, it will break existing behaviour when user was able to remove these inserted options like this `CompileFlags: {Remove: ['--driver-mode=*', '--target=*']}`. If I move call of `addTargetAndModeForProgramName` after `SystemIncludeExtractor`r, inserted options will stay in list. User may only override it with `CompileFlags.Compiler` but it will override all compilers. I don't know is it real problem or not so if you think I should still move it after `SystemIncludeExtractor`, I'll do it. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D143436/new/ https://reviews.llvm.org/D143436 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
