[PATCH] D70222: [clangd] Add support for .rsp files in compile_commands.json

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.


================
Comment at: clang/lib/Tooling/ExpandResponseFilesCompilationDatabase.cpp:28
+      llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> FS)
+      : Base(std::move(Base)), Tokenizer(Tokenizer), FS(FS) {
+    assert(this->Base != nullptr);
----------------
nit: `FS(std::move(FS))`


================
Comment at: clang/lib/Tooling/ExpandResponseFilesCompilationDatabase.cpp:49
+  std::vector<CompileCommand> expand(std::vector<CompileCommand> Cmds) const {
+    llvm::ErrorOr<std::string> PreWorkingDirectory =
+        FS->getCurrentWorkingDirectory();
----------------
no need to restore/save, `ExpandResponseFilesDatabase` should have a FS that's not shared with others.

it is iffy anyway, because if initial working directory is faulty, we'll mutate it to the working directory of the last compile command.
or even if it wasn't faulty, someone could delete the directory before we can restore, or someone can change it while we are in the middle of `ExpandResponseFile` calls.


================
Comment at: clang/lib/Tooling/ExpandResponseFilesCompilationDatabase.cpp:61
+        continue;
+      if (FS->setCurrentWorkingDirectory(Cmd.Directory))
+        continue;
----------------
maybe make this check at the beginning of the for loop?

could you also leave a fixme saying that we should rather propagate the current directory into `ExpandResponseFiles` as well in addition to `FS` and someone can take a look at it later on.
That would be more principled than relying on `ExpandResponseFilesDatabase` having its own non-shared copy of `FS`.


================
Comment at: clang/lib/Tooling/JSONCompilationDatabase.cpp:172
+                      inferMissingCompileCommands(expandResponseFiles(
+                          std::move(Base), llvm::vfs::getRealFileSystem())))
                 : nullptr;
----------------
let's change this one to `llvm::vfs::createPhysicalFileSystem` to make sure we are passing a non-shared FS. Unfortunately, `getRealFileSystem` is shared with the whole process.


================
Comment at: clang/unittests/Tooling/CompilationDatabaseTest.cpp:910
+TEST_F(ExpandResponseFilesTest, ExpandResponseFiles) {
+  // clang-format off
+  add("foo.cpp", "clang",
----------------
nit: instead of clang-format off maybe provide command as a raw string literal?


================
Comment at: clang/unittests/Tooling/CompilationDatabaseTest.cpp:912
+  add("foo.cpp", "clang",
+      ("@inner/rsp1.rsp @rsp2.rsp @rsp4.rsp "
+        "@" + RspFileName1 + " @inner/rsp5.rsp @rsp6.rsp")
----------------
this test is checking the functionality of `ExpandFileCommands` helper, which is already tested in `llvm/unittests/Support/CommandLineTest.cpp`

it should be enough to have two simple tests:
- with a compile command that refers to a simple rsp file, and making sure it was expanded.
- with a regular compile command, making sure it stays the same.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70222