kadircet added a comment. > @kadircet it is obvious that there is something in this diff that causes this > hesitancy in accepting it. I'm ready to keep iterating on the solution but I > need a clue what needs the improvement. Please comment.
sorry for the late reply, i was on vacation and just started catching up with things. this looks great, thanks! ================ Comment at: clang-tools-extra/clangd/CompileCommands.cpp:199 + // use/change working directory, which ExpandResponseFiles doesn't). + FS = llvm::vfs::getRealFileSystem(); +} ---------------- getting real filesystem is cheap, no need to make this part of the state, just inline it into `tooling::addExpandedResponseFiles` call. ================ 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: > > > 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. > > @nridge, it will break existing behaviour when user was able to remove > > these inserted options like this `CompileFlags: {Remove: > > ['--driver-mode=*', '--target=*']}`. > > Thanks. I'll leave the final word to @kadircet here but in my opinion, being > able to remove those flags via `Remove` is not important; as you say, the > user can change the `Compiler` to influence the driver mode if needed. agreed. `addTargetAndModeForProgramName` will add these flags only when they're not present. deleting --driver-mode or --target, without providing a fixed replacement doesn't sound like a real use case. ================ Comment at: clang-tools-extra/clangd/test/Inputs/did-change-configuration-params.args:1 +-Wpedantic ---------------- you can test `expandResponseFile` behaviour through unittests as well, see https://github.com/llvm/llvm-project/blob/main/clang-tools-extra/clangd/unittests/CompileCommandsTests.cpp#L133 for an example. i don't think complications in this lit test are worth it. do you mind moving this to a unittest? ================ Comment at: clang-tools-extra/clangd/unittests/ClangdTests.cpp:1240 Server.addDocument(testPath("foo.h"), SourceContents); - ASSERT_TRUE(Server.blockUntilIdleForTest()); + ASSERT_TRUE(Server.blockUntilIdleForTest(/*TimeoutSeconds=*/60)); EXPECT_FALSE(DiagConsumer.HadDiagsInLastCallback); ---------------- this feels unrelated, can you revert? ================ Comment at: clang-tools-extra/clangd/unittests/CompileCommandsTests.cpp:49 tooling::CompileCommand Cmd; Cmd.CommandLine = {"clang++", "--", "foo.cc", "bar.cc"}; Mangler(Cmd, "foo.cc"); ---------------- can you include a target triplet in the compiler name to test out target inference also works as intended? ================ Comment at: clang/lib/Tooling/ExpandResponseFilesCompilationDatabase.cpp:53 + for (auto &Cmd : Cmds) + tooling::addExpandedResponseFiles(Cmd.CommandLine, Cmd.Directory, + Tokenizer, *FS); ---------------- you can drop `tooling::` 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
