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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

mostly LG, a few last nits.



================
Comment at: clang/lib/Tooling/ExpandResponseFilesCompilationDatabase.cpp:51
+      // FIXME: 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.
----------------
nit: drop `and someone can take a look at it later on.`


================
Comment at: clang/unittests/Tooling/CompilationDatabaseTest.cpp:865
+  void SetUp() override {
+    FS = new llvm::vfs::InMemoryFileSystem;
+    ASSERT_TRUE(addFile(path(StringRef("rsp1.rsp")), "-Dflag"));
----------------
make this part of the constructor instead of having a SetUp


================
Comment at: clang/unittests/Tooling/CompilationDatabaseTest.cpp:866
+    FS = new llvm::vfs::InMemoryFileSystem;
+    ASSERT_TRUE(addFile(path(StringRef("rsp1.rsp")), "-Dflag"));
+  }
----------------
move this to the test


================
Comment at: clang/unittests/Tooling/CompilationDatabaseTest.cpp:870
+  bool addFile(StringRef File, StringRef Context) {
+    return FS->addFile(File, 0, llvm::MemoryBuffer::getMemBufferCopy(Context));
+  }
----------------
nit: I would rather keep ASSERT_TRUE in here


================
Comment at: clang/unittests/Tooling/CompilationDatabaseTest.cpp:877
+                       ->getCompileCommands(path(F));
+    if (Results.empty()) {
+      return "none";
----------------
nit: no need for braces


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70222