[PATCH] D143436: [clangd] Move standard options adaptor to CommandMangler

Thu Mar 9 07:03:17 PST 2023

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