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

[liu hui via Phabricator via cfe-commits]

lh123 updated this revision to Diff 231393.
lh123 marked 5 inline comments as done.
lh123 added a comment.

address comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70222

Files:
  clang/include/clang/Tooling/CompilationDatabase.h
  clang/lib/Tooling/CMakeLists.txt
  clang/lib/Tooling/ExpandResponseFilesCompilationDatabase.cpp
  clang/lib/Tooling/JSONCompilationDatabase.cpp
  clang/unittests/Tooling/CompilationDatabaseTest.cpp

Index: clang/unittests/Tooling/CompilationDatabaseTest.cpp
===
--- clang/unittests/Tooling/CompilationDatabaseTest.cpp
+++ clang/unittests/Tooling/CompilationDatabaseTest.cpp
@@ -859,5 +859,35 @@
 "clang++ --driver-mode=g++ bar.cpp -D bar.cpp");
 }
 
+class ExpandResponseFilesTest : public MemDBTest {
+public:
+  ExpandResponseFilesTest() : FS(new llvm::vfs::InMemoryFileSystem) {}
+
+protected:
+  void addFile(StringRef File, StringRef Content) {
+ASSERT_TRUE(
+FS->addFile(File, 0, llvm::MemoryBuffer::getMemBufferCopy(Content)));
+  }
+
+  std::string getCommand(llvm::StringRef F) {
+auto Results = expandResponseFiles(std::make_unique(Entries), FS)
+   ->getCompileCommands(path(F));
+if (Results.empty())
+  return "none";
+return llvm::join(Results[0].CommandLine, " ");
+  }
+
+  llvm::IntrusiveRefCntPtr FS;
+};
+
+TEST_F(ExpandResponseFilesTest, ExpandResponseFiles) {
+  addFile(path(StringRef("rsp1.rsp")), "-Dflag");
+
+  add("foo.cpp", "clang", "@rsp1.rsp");
+  add("bar.cpp", "clang", "-Dflag");
+  EXPECT_EQ(getCommand("foo.cpp"), "clang foo.cpp -D foo.cpp -Dflag");
+  EXPECT_EQ(getCommand("bar.cpp"), "clang bar.cpp -D bar.cpp -Dflag");
+}
+
 } // end namespace tooling
 } // end namespace clang
Index: clang/lib/Tooling/JSONCompilationDatabase.cpp
===
--- clang/lib/Tooling/JSONCompilationDatabase.cpp
+++ clang/lib/Tooling/JSONCompilationDatabase.cpp
@@ -168,7 +168,9 @@
 auto Base = JSONCompilationDatabase::loadFromFile(
 JSONDatabasePath, ErrorMessage, JSONCommandLineSyntax::AutoDetect);
 return Base ? inferTargetAndDriverMode(
-  inferMissingCompileCommands(std::move(Base)))
+  inferMissingCompileCommands(expandResponseFiles(
+  std::move(Base),
+  llvm::vfs::createPhysicalFileSystem().release(
 : nullptr;
   }
 };
Index: clang/lib/Tooling/ExpandResponseFilesCompilationDatabase.cpp
===
--- /dev/null
+++ clang/lib/Tooling/ExpandResponseFilesCompilationDatabase.cpp
@@ -0,0 +1,92 @@
+//===- ExpandResponseFileCompilationDataBase.cpp --===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "clang/Tooling/CompilationDatabase.h"
+#include "llvm/ADT/Triple.h"
+#include "llvm/Support/CommandLine.h"
+#include "llvm/Support/ConvertUTF.h"
+#include "llvm/Support/ErrorOr.h"
+#include "llvm/Support/MemoryBuffer.h"
+#include "llvm/Support/Path.h"
+#include "llvm/Support/StringSaver.h"
+
+namespace clang {
+namespace tooling {
+namespace {
+
+class ExpandResponseFilesDatabase : public CompilationDatabase {
+public:
+  ExpandResponseFilesDatabase(
+  std::unique_ptr Base,
+  llvm::cl::TokenizerCallback Tokenizer,
+  llvm::IntrusiveRefCntPtr FS)
+  : Base(std::move(Base)), Tokenizer(Tokenizer), FS(std::move(FS)) {
+assert(this->Base != nullptr);
+assert(this->Tokenizer != nullptr);
+assert(this->FS != nullptr);
+  }
+
+  std::vector getAllFiles() const override {
+return Base->getAllFiles();
+  }
+
+  std::vector
+  getCompileCommands(StringRef FilePath) const override {
+return expand(Base->getCompileCommands(FilePath));
+  }
+
+  std::vector getAllCompileCommands() const override {
+return expand(Base->getAllCompileCommands());
+  }
+
+private:
+  std::vector expand(std::vector Cmds) const {
+for (auto  : Cmds) {
+  // FIXME: we should rather propagate the current directory into
+  // ExpandResponseFiles as well in addition to FS.
+  if (std::error_code EC = FS->setCurrentWorkingDirectory(Cmd.Directory)) {
+llvm::consumeError(llvm::errorCodeToError(EC));
+continue;
+  }
+  bool SeenRSPFile = false;
+  llvm::SmallVector Argv;
+  Argv.reserve(Cmd.CommandLine.size());
+  for (auto  : Cmd.CommandLine) {
+Argv.push_back(Arg.c_str());
+SeenRSPFile |= Arg.front() == '@';
+  }
+  if (!SeenRSPFile)
+continue;
+  llvm::BumpPtrAllocator Alloc;

[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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[pre-merge checks [bot] via Phabricator via cfe-commits]

merge_guards_bot added a comment.

Build result: FAILURE - 
Log files: console-log.txt 
,
 CMakeCache.txt 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70222



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[liu hui via Phabricator via cfe-commits]

lh123 updated this revision to Diff 231357.
lh123 marked 6 inline comments as done.
lh123 added a comment.

address comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70222

Files:
  clang/include/clang/Tooling/CompilationDatabase.h
  clang/lib/Tooling/CMakeLists.txt
  clang/lib/Tooling/ExpandResponseFilesCompilationDatabase.cpp
  clang/lib/Tooling/JSONCompilationDatabase.cpp
  clang/unittests/Tooling/CompilationDatabaseTest.cpp

Index: clang/unittests/Tooling/CompilationDatabaseTest.cpp
===
--- clang/unittests/Tooling/CompilationDatabaseTest.cpp
+++ clang/unittests/Tooling/CompilationDatabaseTest.cpp
@@ -859,5 +859,36 @@
 "clang++ --driver-mode=g++ bar.cpp -D bar.cpp");
 }
 
+class ExpandResponseFilesTest : public MemDBTest {
+protected:
+  void SetUp() override {
+FS = new llvm::vfs::InMemoryFileSystem;
+ASSERT_TRUE(addFile(path(StringRef("rsp1.rsp")), "-Dflag"));
+  }
+
+  bool addFile(StringRef File, StringRef Context) {
+return FS->addFile(File, 0, llvm::MemoryBuffer::getMemBufferCopy(Context));
+  }
+
+  std::string getCommand(llvm::StringRef F) {
+auto Results = expandResponseFiles(std::make_unique(Entries),
+   FS)
+   ->getCompileCommands(path(F));
+if (Results.empty()) {
+  return "none";
+}
+return llvm::join(Results[0].CommandLine, " ");
+  }
+
+  llvm::IntrusiveRefCntPtr FS;
+};
+
+TEST_F(ExpandResponseFilesTest, ExpandResponseFiles) {
+  add("foo.cpp", "clang", "@rsp1.rsp");
+  add("bar.cpp", "clang", "-Dflag");
+  EXPECT_EQ(getCommand("foo.cpp"), "clang foo.cpp -D foo.cpp -Dflag");
+  EXPECT_EQ(getCommand("bar.cpp"), "clang bar.cpp -D bar.cpp -Dflag");
+}
+
 } // end namespace tooling
 } // end namespace clang
Index: clang/lib/Tooling/JSONCompilationDatabase.cpp
===
--- clang/lib/Tooling/JSONCompilationDatabase.cpp
+++ clang/lib/Tooling/JSONCompilationDatabase.cpp
@@ -168,7 +168,9 @@
 auto Base = JSONCompilationDatabase::loadFromFile(
 JSONDatabasePath, ErrorMessage, JSONCommandLineSyntax::AutoDetect);
 return Base ? inferTargetAndDriverMode(
-  inferMissingCompileCommands(std::move(Base)))
+  inferMissingCompileCommands(expandResponseFiles(
+  std::move(Base),
+  llvm::vfs::createPhysicalFileSystem().get(
 : nullptr;
   }
 };
Index: clang/lib/Tooling/ExpandResponseFilesCompilationDatabase.cpp
===
--- /dev/null
+++ clang/lib/Tooling/ExpandResponseFilesCompilationDatabase.cpp
@@ -0,0 +1,93 @@
+//===- ExpandResponseFileCompilationDataBase.cpp --===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "clang/Tooling/CompilationDatabase.h"
+#include "llvm/ADT/Triple.h"
+#include "llvm/Support/CommandLine.h"
+#include "llvm/Support/ConvertUTF.h"
+#include "llvm/Support/ErrorOr.h"
+#include "llvm/Support/MemoryBuffer.h"
+#include "llvm/Support/Path.h"
+#include "llvm/Support/StringSaver.h"
+
+namespace clang {
+namespace tooling {
+namespace {
+
+class ExpandResponseFilesDatabase : public CompilationDatabase {
+public:
+  ExpandResponseFilesDatabase(
+  std::unique_ptr Base,
+  llvm::cl::TokenizerCallback Tokenizer,
+  llvm::IntrusiveRefCntPtr FS)
+  : Base(std::move(Base)), Tokenizer(Tokenizer), FS(std::move(FS)) {
+assert(this->Base != nullptr);
+assert(this->Tokenizer != nullptr);
+assert(this->FS != nullptr);
+  }
+
+  std::vector getAllFiles() const override {
+return Base->getAllFiles();
+  }
+
+  std::vector
+  getCompileCommands(StringRef FilePath) const override {
+return expand(Base->getCompileCommands(FilePath));
+  }
+
+  std::vector getAllCompileCommands() const override {
+return expand(Base->getAllCompileCommands());
+  }
+
+private:
+  std::vector expand(std::vector Cmds) const {
+for (auto  : Cmds) {
+  // 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.
+  if (std::error_code EC = FS->setCurrentWorkingDirectory(Cmd.Directory)) {
+llvm::consumeError(llvm::errorCodeToError(EC));
+continue;
+  }
+  bool SeenRSPFile = false;
+  llvm::SmallVector Argv;
+  Argv.reserve(Cmd.CommandLine.size());
+  for (auto  : Cmd.CommandLine) {
+Argv.push_back(Arg.c_str());
+SeenRSPFile |= Arg.front() == 

[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 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 expand(std::vector Cmds) const {
+llvm::ErrorOr 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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[pre-merge checks [bot] via Phabricator via cfe-commits]

merge_guards_bot added a comment.

Build result: FAILURE - 
Log files: console-log.txt 
,
 CMakeCache.txt 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70222



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[liu hui via Phabricator via cfe-commits]

lh123 updated this revision to Diff 231235.
lh123 added a comment.

rebase


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70222

Files:
  clang/include/clang/Tooling/CompilationDatabase.h
  clang/lib/Tooling/CMakeLists.txt
  clang/lib/Tooling/ExpandResponseFilesCompilationDatabase.cpp
  clang/lib/Tooling/JSONCompilationDatabase.cpp
  clang/unittests/Tooling/CompilationDatabaseTest.cpp

Index: clang/unittests/Tooling/CompilationDatabaseTest.cpp
===
--- clang/unittests/Tooling/CompilationDatabaseTest.cpp
+++ clang/unittests/Tooling/CompilationDatabaseTest.cpp
@@ -859,5 +859,64 @@
 "clang++ --driver-mode=g++ bar.cpp -D bar.cpp");
 }
 
+class ExpandResponseFilesTest : public MemDBTest {
+protected:
+  void SetUp() override {
+FS = new llvm::vfs::InMemoryFileSystem;
+
+InnerDir = path(StringRef("inner"));
+
+llvm::sys::path::append(RspFileName1, InnerDir, "rsp1.rsp");
+addFile(RspFileName1, "-Dflag1");
+
+RspFileName2 = path(StringRef("rsp2.rsp"));
+addFile(RspFileName2, "-Dflag2 @rsp3.rsp");
+
+RspFileName3 = path(StringRef("rsp3.rsp"));
+addFile(RspFileName3, "-Dflag3");
+
+RspFileName4 = path(StringRef("rsp4.rsp"));
+addFile(RspFileName4, "-Dflag4 @rsp4.rsp");
+
+llvm::sys::path::append(RspFileName5, InnerDir, "rsp5.rsp");
+addFile(RspFileName5, "-Dflag5 @inner/rsp1.rsp");
+  }
+
+  void addFile(StringRef File, StringRef Context) {
+ASSERT_TRUE(FS->addFile(File, 0, llvm::MemoryBuffer::getMemBufferCopy(Context)));
+  }
+
+  std::string getCommand(llvm::StringRef F) {
+auto Results = expandResponseFiles(std::make_unique(Entries),
+   FS)
+   ->getCompileCommands(path(F));
+if (Results.empty()) {
+  return "none";
+}
+return llvm::join(Results[0].CommandLine, " ");
+  }
+
+  SmallString<128> InnerDir;
+  SmallString<128> RspFileName1;
+  SmallString<128> RspFileName2;
+  SmallString<128> RspFileName3;
+  SmallString<128> RspFileName4;
+  SmallString<128> RspFileName5;
+  llvm::IntrusiveRefCntPtr FS;
+
+};
+
+TEST_F(ExpandResponseFilesTest, ExpandResponseFiles) {
+  // clang-format off
+  add("foo.cpp", "clang",
+  ("@inner/rsp1.rsp @rsp2.rsp @rsp4.rsp "
+"@" + RspFileName1 + " @inner/rsp5.rsp @rsp6.rsp")
+  .str());
+  // clang-format on
+  EXPECT_EQ(getCommand("foo.cpp"), "clang foo.cpp -D foo.cpp -Dflag1 -Dflag2 "
+   "-Dflag3 -Dflag4 @rsp4.rsp -Dflag1 "
+   "-Dflag5 -Dflag1 @rsp6.rsp");
+}
+
 } // end namespace tooling
 } // end namespace clang
Index: clang/lib/Tooling/JSONCompilationDatabase.cpp
===
--- clang/lib/Tooling/JSONCompilationDatabase.cpp
+++ clang/lib/Tooling/JSONCompilationDatabase.cpp
@@ -168,7 +168,8 @@
 auto Base = JSONCompilationDatabase::loadFromFile(
 JSONDatabasePath, ErrorMessage, JSONCommandLineSyntax::AutoDetect);
 return Base ? inferTargetAndDriverMode(
-  inferMissingCompileCommands(std::move(Base)))
+  inferMissingCompileCommands(expandResponseFiles(
+  std::move(Base), llvm::vfs::getRealFileSystem(
 : nullptr;
   }
 };
Index: clang/lib/Tooling/ExpandResponseFilesCompilationDatabase.cpp
===
--- /dev/null
+++ clang/lib/Tooling/ExpandResponseFilesCompilationDatabase.cpp
@@ -0,0 +1,92 @@
+//===- ExpandResponseFileCompilationDataBase.cpp --===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "clang/Tooling/CompilationDatabase.h"
+#include "llvm/ADT/Triple.h"
+#include "llvm/Support/CommandLine.h"
+#include "llvm/Support/ConvertUTF.h"
+#include "llvm/Support/ErrorOr.h"
+#include "llvm/Support/MemoryBuffer.h"
+#include "llvm/Support/Path.h"
+#include "llvm/Support/StringSaver.h"
+
+namespace clang {
+namespace tooling {
+namespace {
+
+class ExpandResponseFilesDatabase : public CompilationDatabase {
+public:
+  ExpandResponseFilesDatabase(
+  std::unique_ptr Base,
+  llvm::cl::TokenizerCallback Tokenizer,
+  llvm::IntrusiveRefCntPtr FS)
+  : Base(std::move(Base)), Tokenizer(Tokenizer), FS(FS) {
+assert(this->Base != nullptr);
+assert(this->Tokenizer != nullptr);
+assert(this->FS != nullptr);
+  }
+
+  std::vector getAllFiles() const override {
+return Base->getAllFiles();
+  }
+
+  std::vector
+  getCompileCommands(StringRef FilePath) const override {
+return 

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

[pre-merge checks [bot] via Phabricator via cfe-commits]

merge_guards_bot added a comment.

Build result: fail - 60344 tests passed, 2 failed and 732 were skipped.

  failed: Clang.CodeGen/catch-implicit-integer-sign-changes-incdec.c
  failed: Clang.CodeGen/catch-implicit-signed-integer-truncations-incdec.c

Log files: console-log.txt 
,
 CMakeCache.txt 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70222



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[liu hui via Phabricator via cfe-commits]

lh123 updated this revision to Diff 231217.
lh123 set the repository for this revision to rG LLVM Github Monorepo.
lh123 added a comment.

rebase to D70769 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70222

Files:
  clang/include/clang/Tooling/CompilationDatabase.h
  clang/lib/Tooling/CMakeLists.txt
  clang/lib/Tooling/ExpandResponseFilesCompilationDatabase.cpp
  clang/lib/Tooling/JSONCompilationDatabase.cpp
  clang/unittests/Tooling/CompilationDatabaseTest.cpp

Index: clang/unittests/Tooling/CompilationDatabaseTest.cpp
===
--- clang/unittests/Tooling/CompilationDatabaseTest.cpp
+++ clang/unittests/Tooling/CompilationDatabaseTest.cpp
@@ -859,5 +859,64 @@
 "clang++ --driver-mode=g++ bar.cpp -D bar.cpp");
 }
 
+class ExpandResponseFilesTest : public MemDBTest {
+protected:
+  void SetUp() override {
+FS = new llvm::vfs::InMemoryFileSystem;
+
+InnerDir = path(StringRef("inner"));
+
+llvm::sys::path::append(RspFileName1, InnerDir, "rsp1.rsp");
+addFile(RspFileName1, "-Dflag1");
+
+RspFileName2 = path(StringRef("rsp2.rsp"));
+addFile(RspFileName2, "-Dflag2 @rsp3.rsp");
+
+RspFileName3 = path(StringRef("rsp3.rsp"));
+addFile(RspFileName3, "-Dflag3");
+
+RspFileName4 = path(StringRef("rsp4.rsp"));
+addFile(RspFileName4, "-Dflag4 @rsp4.rsp");
+
+llvm::sys::path::append(RspFileName5, InnerDir, "rsp5.rsp");
+addFile(RspFileName5, "-Dflag5 @inner/rsp1.rsp");
+  }
+
+  void addFile(StringRef File, StringRef Context) {
+ASSERT_TRUE(FS->addFile(File, 0, llvm::MemoryBuffer::getMemBufferCopy(Context)));
+  }
+
+  std::string getCommand(llvm::StringRef F) {
+auto Results = expandResponseFiles(std::make_unique(Entries),
+   FS)
+   ->getCompileCommands(path(F));
+if (Results.empty()) {
+  return "none";
+}
+return llvm::join(Results[0].CommandLine, " ");
+  }
+
+  SmallString<128> InnerDir;
+  SmallString<128> RspFileName1;
+  SmallString<128> RspFileName2;
+  SmallString<128> RspFileName3;
+  SmallString<128> RspFileName4;
+  SmallString<128> RspFileName5;
+  llvm::IntrusiveRefCntPtr FS;
+
+};
+
+TEST_F(ExpandResponseFilesTest, ExpandResponseFiles) {
+  // clang-format off
+  add("foo.cpp", "clang",
+  ("@inner/rsp1.rsp @rsp2.rsp @rsp4.rsp "
+"@" + RspFileName1 + " @inner/rsp5.rsp @rsp6.rsp")
+  .str());
+  // clang-format on
+  EXPECT_EQ(getCommand("foo.cpp"), "clang foo.cpp -D foo.cpp -Dflag1 -Dflag2 "
+   "-Dflag3 -Dflag4 @rsp4.rsp -Dflag1 "
+   "-Dflag5 -Dflag1 @rsp6.rsp");
+}
+
 } // end namespace tooling
 } // end namespace clang
Index: clang/lib/Tooling/JSONCompilationDatabase.cpp
===
--- clang/lib/Tooling/JSONCompilationDatabase.cpp
+++ clang/lib/Tooling/JSONCompilationDatabase.cpp
@@ -168,7 +168,8 @@
 auto Base = JSONCompilationDatabase::loadFromFile(
 JSONDatabasePath, ErrorMessage, JSONCommandLineSyntax::AutoDetect);
 return Base ? inferTargetAndDriverMode(
-  inferMissingCompileCommands(std::move(Base)))
+  inferMissingCompileCommands(expandResponseFiles(
+  std::move(Base), llvm::vfs::getRealFileSystem(
 : nullptr;
   }
 };
Index: clang/lib/Tooling/ExpandResponseFilesCompilationDatabase.cpp
===
--- /dev/null
+++ clang/lib/Tooling/ExpandResponseFilesCompilationDatabase.cpp
@@ -0,0 +1,92 @@
+//===- ExpandResponseFileCompilationDataBase.cpp --===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "clang/Tooling/CompilationDatabase.h"
+#include "llvm/ADT/Triple.h"
+#include "llvm/Support/CommandLine.h"
+#include "llvm/Support/ConvertUTF.h"
+#include "llvm/Support/ErrorOr.h"
+#include "llvm/Support/MemoryBuffer.h"
+#include "llvm/Support/Path.h"
+#include "llvm/Support/StringSaver.h"
+
+namespace clang {
+namespace tooling {
+namespace {
+
+class ExpandResponseFilesDatabase : public CompilationDatabase {
+public:
+  ExpandResponseFilesDatabase(
+  std::unique_ptr Base,
+  llvm::cl::TokenizerCallback Tokenizer,
+  llvm::IntrusiveRefCntPtr FS)
+  : Base(std::move(Base)), Tokenizer(Tokenizer), FS(FS) {
+assert(this->Base != nullptr);
+assert(this->Tokenizer != nullptr);
+assert(this->FS != nullptr);
+  }
+
+  std::vector getAllFiles() const override {
+return Base->getAllFiles();

[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:167
+for (auto  : Cmds) {
+  expandResponseFiles(Cmd, Tokenizer);
+}

lh123 wrote:
> lh123 wrote:
> > lh123 wrote:
> > > kadircet wrote:
> > > > so it looks like we already have `ExpandResponseFiles` exposed in 
> > > > `llvm/include/llvm/Support/CommandLine.h` could you make use of it in 
> > > > here instead of re-implementing it again?
> > > > 
> > > > I see that it has a different signature, but we can do the conversion 
> > > > back and forth in here, going from `std::string` to `char*` is cheap 
> > > > anyways, coming back is expensive though, and we can limit that to iff 
> > > > we have seen an argument starting with an `@`. So this would be 
> > > > something like:
> > > > 
> > > > ```
> > > > llvm::SmallVector Argvs;
> > > > Argvs.reserve(Cmd.CommandLine.size());
> > > > bool SeenRSPFile = false;
> > > > for(auto  : Cmd.CommandLine) {
> > > >   Argvs.push_back(Argv.c_str());
> > > >   SeenRSPFile |= Argv.front() == '@';
> > > > }
> > > > if(!SeenRSPFile)
> > > >   continue;
> > > > 
> > > > // call ExpandResponseFiles with Argvs, convert back to 
> > > > vector and assign to Cmd.CommandLine
> > > > ```
> > > I think it's not easy to reuse `ExpandResponseFiles` without changing 
> > > code because it uses llvm::sys::fs::current_path to resolve relative 
> > > paths.
> > > 
> > > In fact, most of the code here is copied from `CommandLine`
> > But we can reuse `static bool ExpandResponseFile(StringRef FName, 
> > StringSaver , TokenizerCallback Tokenizer, SmallVectorImpl > *> , bool MarkEOLs, bool RelativeNames)` if we expose it in 
> > `CommandLine.h`
> `static bool ExpandResponseFile(StringRef FName, StringSaver , 
> TokenizerCallback Tokenizer, SmallVectorImpl , bool 
> MarkEOLs, bool RelativeNames)` also seems to use 
> `llvm::sys::fs::current_path` to resolve relative paths.
> I think it's not easy to reuse ExpandResponseFiles without changing code 
> because it uses llvm::sys::fs::current_path to resolve relative paths.

Ah, I missed this, thanks for pointing it out.

> In fact, most of the code here is copied from CommandLine

I believe in such a scenario, the correct course of action would be to update 
`ExpandResponseFiles` in `CommandLine.h` to accept a `FileSystem `, instead 
of duplicating the logic.
Namely changing the signature to:

```
bool ExpandResponseFiles(StringSaver , TokenizerCallback Tokenizer,
 llvm::vfs::FileSystem , SmallVectorImpl ,
 bool MarkEOLs = false, bool RelativeNames = false);
```

Then you can pass `llvm::vfs::getRealFileSystem()` to these function in 
existing call sites.
And create a FS with working directory set to `Cmd.Directory` via 
`FileSystem::setCurrentWorkingDirectory` in this specific call site you want to 
introduce.

Also to update the implementation of `ExpandResponseFiles` you would need to 
make use of `FileSystem::getCurrentWorkingDirectory()` instead of 
`llvm::sys::fs::current_path` and `FileSystem::getBufferForFile()` instead of 
`llvm::MemoryBuffer::getFile` the rest shouldn't need any changes hopefully, 
except plumbing VFS to some helpers.

Please send the signature change and call site updates in a separate patch 
though.


After that patch you can change signature for 
`std::unique_ptr 
expandResponseFiles(std::unique_ptr Base)` to accept a 
`IntrusiveRefCntPtr VFS`, which will be `getRealFileSystem` in case 
of JSONCompilationDatabase. But this will enable you to pass a 
`InMemoryFileSystem` in the unittest below.


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

https://reviews.llvm.org/D70222



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[liu hui via Phabricator via cfe-commits]

lh123 marked an inline comment as done.
lh123 added inline comments.



Comment at: clang/lib/Tooling/ExpandResponseFilesCompilationDatabase.cpp:167
+for (auto  : Cmds) {
+  expandResponseFiles(Cmd, Tokenizer);
+}

lh123 wrote:
> lh123 wrote:
> > kadircet wrote:
> > > so it looks like we already have `ExpandResponseFiles` exposed in 
> > > `llvm/include/llvm/Support/CommandLine.h` could you make use of it in 
> > > here instead of re-implementing it again?
> > > 
> > > I see that it has a different signature, but we can do the conversion 
> > > back and forth in here, going from `std::string` to `char*` is cheap 
> > > anyways, coming back is expensive though, and we can limit that to iff we 
> > > have seen an argument starting with an `@`. So this would be something 
> > > like:
> > > 
> > > ```
> > > llvm::SmallVector Argvs;
> > > Argvs.reserve(Cmd.CommandLine.size());
> > > bool SeenRSPFile = false;
> > > for(auto  : Cmd.CommandLine) {
> > >   Argvs.push_back(Argv.c_str());
> > >   SeenRSPFile |= Argv.front() == '@';
> > > }
> > > if(!SeenRSPFile)
> > >   continue;
> > > 
> > > // call ExpandResponseFiles with Argvs, convert back to 
> > > vector and assign to Cmd.CommandLine
> > > ```
> > I think it's not easy to reuse `ExpandResponseFiles` without changing code 
> > because it uses llvm::sys::fs::current_path to resolve relative paths.
> > 
> > In fact, most of the code here is copied from `CommandLine`
> But we can reuse `static bool ExpandResponseFile(StringRef FName, StringSaver 
> , TokenizerCallback Tokenizer, SmallVectorImpl , 
> bool MarkEOLs, bool RelativeNames)` if we expose it in `CommandLine.h`
`static bool ExpandResponseFile(StringRef FName, StringSaver , 
TokenizerCallback Tokenizer, SmallVectorImpl , bool 
MarkEOLs, bool RelativeNames)` also seems to use `llvm::sys::fs::current_path` 
to resolve relative paths.


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

https://reviews.llvm.org/D70222



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[liu hui via Phabricator via cfe-commits]

lh123 marked an inline comment as done.
lh123 added inline comments.



Comment at: clang/lib/Tooling/ExpandResponseFilesCompilationDatabase.cpp:167
+for (auto  : Cmds) {
+  expandResponseFiles(Cmd, Tokenizer);
+}

lh123 wrote:
> kadircet wrote:
> > so it looks like we already have `ExpandResponseFiles` exposed in 
> > `llvm/include/llvm/Support/CommandLine.h` could you make use of it in here 
> > instead of re-implementing it again?
> > 
> > I see that it has a different signature, but we can do the conversion back 
> > and forth in here, going from `std::string` to `char*` is cheap anyways, 
> > coming back is expensive though, and we can limit that to iff we have seen 
> > an argument starting with an `@`. So this would be something like:
> > 
> > ```
> > llvm::SmallVector Argvs;
> > Argvs.reserve(Cmd.CommandLine.size());
> > bool SeenRSPFile = false;
> > for(auto  : Cmd.CommandLine) {
> >   Argvs.push_back(Argv.c_str());
> >   SeenRSPFile |= Argv.front() == '@';
> > }
> > if(!SeenRSPFile)
> >   continue;
> > 
> > // call ExpandResponseFiles with Argvs, convert back to vector 
> > and assign to Cmd.CommandLine
> > ```
> I think it's not easy to reuse `ExpandResponseFiles` without changing code 
> because it uses llvm::sys::fs::current_path to resolve relative paths.
> 
> In fact, most of the code here is copied from `CommandLine`
But we can reuse `static bool ExpandResponseFile(StringRef FName, StringSaver 
, TokenizerCallback Tokenizer, SmallVectorImpl , 
bool MarkEOLs, bool RelativeNames)` if we expose it in `CommandLine.h`


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

https://reviews.llvm.org/D70222



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[liu hui via Phabricator via cfe-commits]

lh123 updated this revision to Diff 231051.
lh123 added a comment.

address comments


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

https://reviews.llvm.org/D70222

Files:
  clang/include/clang/Tooling/CompilationDatabase.h
  clang/lib/Tooling/CMakeLists.txt
  clang/lib/Tooling/ExpandResponseFilesCompilationDatabase.cpp
  clang/lib/Tooling/JSONCompilationDatabase.cpp
  clang/unittests/Tooling/CompilationDatabaseTest.cpp

Index: clang/unittests/Tooling/CompilationDatabaseTest.cpp
===
--- clang/unittests/Tooling/CompilationDatabaseTest.cpp
+++ clang/unittests/Tooling/CompilationDatabaseTest.cpp
@@ -17,6 +17,7 @@
 #include "llvm/Support/TargetSelect.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
+#include 
 
 namespace clang {
 namespace tooling {
@@ -859,5 +860,76 @@
 "clang++ --driver-mode=g++ bar.cpp -D bar.cpp");
 }
 
+class ExpandResponseFilesTest : public MemDBTest {
+protected:
+  void SetUp() override {
+InnerDir = path(StringRef("inner"));
+std::error_code EC = llvm::sys::fs::create_directory(InnerDir);
+EXPECT_TRUE(!EC);
+
+llvm::sys::path::append(RspFileName1, InnerDir, "rsp1.rsp");
+std::ofstream RspFile1(RspFileName1.c_str());
+RspFile1 << "-Dflag1";
+RspFile1.close();
+
+RspFileName2 = path(StringRef("rsp2.rsp"));
+std::ofstream RspFile2(RspFileName2.c_str());
+RspFile2 << "-Dflag2 @rsp3.rsp";
+RspFile2.close();
+
+RspFileName3 = path(StringRef("rsp3.rsp"));
+std::ofstream RspFile3(RspFileName3.c_str());
+RspFile3 << "-Dflag3";
+RspFile3.close();
+
+RspFileName4 = path(StringRef("rsp4.rsp"));
+std::ofstream  RspFile4(RspFileName4.c_str());
+RspFile4 << "-Dflag4 @rsp4.rsp";
+RspFile4.close();
+
+llvm::sys::path::append(RspFileName5, InnerDir, "rsp5.rsp");
+std::ofstream RspFile5(RspFileName5.c_str());
+RspFile5 << "-Dflag5 @inner/rsp1.rsp";
+RspFile5.close();
+  }
+
+  void TearDown() override {
+llvm::sys::fs::remove(RspFileName5);
+llvm::sys::fs::remove(RspFileName4);
+llvm::sys::fs::remove(RspFileName3);
+llvm::sys::fs::remove(RspFileName2);
+llvm::sys::fs::remove(RspFileName1);
+llvm::sys::fs::remove(InnerDir);
+  }
+
+  std::string getCommand(llvm::StringRef F) {
+auto Results = expandResponseFiles(std::make_unique(Entries))
+   ->getCompileCommands(path(F));
+if (Results.empty()) {
+  return "none";
+}
+return llvm::join(Results[0].CommandLine, " ");
+  }
+
+  SmallString<128> InnerDir;
+  SmallString<128> RspFileName1;
+  SmallString<128> RspFileName2;
+  SmallString<128> RspFileName3;
+  SmallString<128> RspFileName4;
+  SmallString<128> RspFileName5;
+};
+
+TEST_F(ExpandResponseFilesTest, ExpandResponseFiles) {
+  // clang-format off
+  add("foo.cpp", "clang",
+  ("@inner/rsp1.rsp @rsp2.rsp @rsp4.rsp "
+"@" + RspFileName1 + " @inner/rsp5.rsp @rsp6.rsp")
+  .str());
+  // clang-format on
+  EXPECT_EQ(getCommand("foo.cpp"), "clang foo.cpp -D foo.cpp -Dflag1 -Dflag2 "
+   "-Dflag3 -Dflag4 @rsp4.rsp -Dflag1 "
+   "-Dflag5 -Dflag1 @rsp6.rsp");
+}
+
 } // end namespace tooling
 } // end namespace clang
Index: clang/lib/Tooling/JSONCompilationDatabase.cpp
===
--- clang/lib/Tooling/JSONCompilationDatabase.cpp
+++ clang/lib/Tooling/JSONCompilationDatabase.cpp
@@ -167,8 +167,8 @@
 llvm::sys::path::append(JSONDatabasePath, "compile_commands.json");
 auto Base = JSONCompilationDatabase::loadFromFile(
 JSONDatabasePath, ErrorMessage, JSONCommandLineSyntax::AutoDetect);
-return Base ? inferTargetAndDriverMode(
-  inferMissingCompileCommands(std::move(Base)))
+return Base ? inferTargetAndDriverMode(inferMissingCompileCommands(
+  expandResponseFiles(std::move(Base
 : nullptr;
   }
 };
Index: clang/lib/Tooling/ExpandResponseFilesCompilationDatabase.cpp
===
--- /dev/null
+++ clang/lib/Tooling/ExpandResponseFilesCompilationDatabase.cpp
@@ -0,0 +1,191 @@
+//===- ExpandResponseFileCompilationDataBase.cpp --===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "clang/Tooling/CompilationDatabase.h"
+#include "llvm/ADT/Triple.h"
+#include "llvm/Support/CommandLine.h"
+#include "llvm/Support/ConvertUTF.h"
+#include "llvm/Support/ErrorOr.h"
+#include "llvm/Support/MemoryBuffer.h"
+#include "llvm/Support/Path.h"
+#include "llvm/Support/StringSaver.h"
+
+namespace clang {
+namespace tooling 

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

[liu hui via Phabricator via cfe-commits]

lh123 marked an inline comment as done.
lh123 added inline comments.



Comment at: clang/lib/Tooling/CompilationDatabase.cpp:402
 llvm::sys::path::append(DatabasePath, "compile_flags.txt");
-return FixedCompilationDatabase::loadFromFile(DatabasePath, ErrorMessage);
+auto Base =
+FixedCompilationDatabase::loadFromFile(DatabasePath, ErrorMessage);

kadircet wrote:
> is it really common for rsp files to show up in fixed compilation databases? 
> since compile_flags.txt itself is also a file doesn't make much sense to 
> refer to another one. as it can also contain text of arbitrary length.
Yes, rsp files usually only appear on the command line.


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

https://reviews.llvm.org/D70222



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[liu hui via Phabricator via cfe-commits]

lh123 marked an inline comment as done.
lh123 added inline comments.



Comment at: clang/lib/Tooling/ExpandResponseFilesCompilationDatabase.cpp:167
+for (auto  : Cmds) {
+  expandResponseFiles(Cmd, Tokenizer);
+}

kadircet wrote:
> so it looks like we already have `ExpandResponseFiles` exposed in 
> `llvm/include/llvm/Support/CommandLine.h` could you make use of it in here 
> instead of re-implementing it again?
> 
> I see that it has a different signature, but we can do the conversion back 
> and forth in here, going from `std::string` to `char*` is cheap anyways, 
> coming back is expensive though, and we can limit that to iff we have seen an 
> argument starting with an `@`. So this would be something like:
> 
> ```
> llvm::SmallVector Argvs;
> Argvs.reserve(Cmd.CommandLine.size());
> bool SeenRSPFile = false;
> for(auto  : Cmd.CommandLine) {
>   Argvs.push_back(Argv.c_str());
>   SeenRSPFile |= Argv.front() == '@';
> }
> if(!SeenRSPFile)
>   continue;
> 
> // call ExpandResponseFiles with Argvs, convert back to vector 
> and assign to Cmd.CommandLine
> ```
I think it's not easy to reuse `ExpandResponseFiles` without changing code 
because it uses llvm::sys::fs::current_path to resolve relative paths.

In fact, most of the code here is copied from `CommandLine`


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

https://reviews.llvm.org/D70222



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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/include/clang/Tooling/CompilationDatabase.h:223
 
+/// Returns a wrapped CompilationDatabase that will expand all response files 
on
+/// commandline returned by underlying database.

nit: `s/response files/rsp(response) files/`



Comment at: clang/include/clang/Tooling/CompilationDatabase.h:230
+expandResponseFiles(std::unique_ptr Base,
+llvm::cl::TokenizerCallback Tokenizer = nullptr);
+

it looks like none of the callsites are setting this parameter can we get rid 
of this one?



Comment at: clang/lib/Tooling/CompilationDatabase.cpp:402
 llvm::sys::path::append(DatabasePath, "compile_flags.txt");
-return FixedCompilationDatabase::loadFromFile(DatabasePath, ErrorMessage);
+auto Base =
+FixedCompilationDatabase::loadFromFile(DatabasePath, ErrorMessage);

is it really common for rsp files to show up in fixed compilation databases? 
since compile_flags.txt itself is also a file doesn't make much sense to refer 
to another one. as it can also contain text of arbitrary length.



Comment at: clang/lib/Tooling/ExpandResponseFilesCompilationDatabase.cpp:167
+for (auto  : Cmds) {
+  expandResponseFiles(Cmd, Tokenizer);
+}

so it looks like we already have `ExpandResponseFiles` exposed in 
`llvm/include/llvm/Support/CommandLine.h` could you make use of it in here 
instead of re-implementing it again?

I see that it has a different signature, but we can do the conversion back and 
forth in here, going from `std::string` to `char*` is cheap anyways, coming 
back is expensive though, and we can limit that to iff we have seen an argument 
starting with an `@`. So this would be something like:

```
llvm::SmallVector Argvs;
Argvs.reserve(Cmd.CommandLine.size());
bool SeenRSPFile = false;
for(auto  : Cmd.CommandLine) {
  Argvs.push_back(Argv.c_str());
  SeenRSPFile |= Argv.front() == '@';
}
if(!SeenRSPFile)
  continue;

// call ExpandResponseFiles with Argvs, convert back to vector and 
assign to Cmd.CommandLine
```


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

https://reviews.llvm.org/D70222



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[liu hui via Phabricator via cfe-commits]

lh123 updated this revision to Diff 230756.
lh123 added a comment.

Expand response files before `inferMissingCompileCommands` and 
`inferTargetAndDriverMode`


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

https://reviews.llvm.org/D70222

Files:
  clang/include/clang/Tooling/CompilationDatabase.h
  clang/lib/Tooling/CMakeLists.txt
  clang/lib/Tooling/CompilationDatabase.cpp
  clang/lib/Tooling/ExpandResponseFilesCompilationDatabase.cpp
  clang/lib/Tooling/JSONCompilationDatabase.cpp
  clang/unittests/Tooling/CompilationDatabaseTest.cpp

Index: clang/unittests/Tooling/CompilationDatabaseTest.cpp
===
--- clang/unittests/Tooling/CompilationDatabaseTest.cpp
+++ clang/unittests/Tooling/CompilationDatabaseTest.cpp
@@ -17,6 +17,7 @@
 #include "llvm/Support/TargetSelect.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
+#include 
 
 namespace clang {
 namespace tooling {
@@ -859,5 +860,76 @@
 "clang++ --driver-mode=g++ bar.cpp -D bar.cpp");
 }
 
+class ExpandResponseFilesTest : public MemDBTest {
+protected:
+  void SetUp() override {
+InnerDir = path(StringRef("inner"));
+std::error_code EC = llvm::sys::fs::create_directory(InnerDir);
+EXPECT_TRUE(!EC);
+
+llvm::sys::path::append(RspFileName1, InnerDir, "rsp1.rsp");
+std::ofstream RspFile1(RspFileName1.c_str());
+RspFile1 << "-Dflag1";
+RspFile1.close();
+
+RspFileName2 = path(StringRef("rsp2.rsp"));
+std::ofstream RspFile2(RspFileName2.c_str());
+RspFile2 << "-Dflag2 @rsp3.rsp";
+RspFile2.close();
+
+RspFileName3 = path(StringRef("rsp3.rsp"));
+std::ofstream RspFile3(RspFileName3.c_str());
+RspFile3 << "-Dflag3";
+RspFile3.close();
+
+RspFileName4 = path(StringRef("rsp4.rsp"));
+std::ofstream  RspFile4(RspFileName4.c_str());
+RspFile4 << "-Dflag4 @rsp4.rsp";
+RspFile4.close();
+
+llvm::sys::path::append(RspFileName5, InnerDir, "rsp5.rsp");
+std::ofstream RspFile5(RspFileName5.c_str());
+RspFile5 << "-Dflag5 @inner/rsp1.rsp";
+RspFile5.close();
+  }
+
+  void TearDown() override {
+llvm::sys::fs::remove(RspFileName5);
+llvm::sys::fs::remove(RspFileName4);
+llvm::sys::fs::remove(RspFileName3);
+llvm::sys::fs::remove(RspFileName2);
+llvm::sys::fs::remove(RspFileName1);
+llvm::sys::fs::remove(InnerDir);
+  }
+
+  std::string getCommand(llvm::StringRef F) {
+auto Results = expandResponseFiles(std::make_unique(Entries))
+   ->getCompileCommands(path(F));
+if (Results.empty()) {
+  return "none";
+}
+return llvm::join(Results[0].CommandLine, " ");
+  }
+
+  SmallString<128> InnerDir;
+  SmallString<128> RspFileName1;
+  SmallString<128> RspFileName2;
+  SmallString<128> RspFileName3;
+  SmallString<128> RspFileName4;
+  SmallString<128> RspFileName5;
+};
+
+TEST_F(ExpandResponseFilesTest, ExpandResponseFiles) {
+  // clang-format off
+  add("foo.cpp", "clang",
+  ("@inner/rsp1.rsp @rsp2.rsp @rsp4.rsp "
+"@" + RspFileName1 + " @inner/rsp5.rsp @rsp6.rsp")
+  .str());
+  // clang-format on
+  EXPECT_EQ(getCommand("foo.cpp"), "clang foo.cpp -D foo.cpp -Dflag1 -Dflag2 "
+   "-Dflag3 -Dflag4 @rsp4.rsp -Dflag1 "
+   "-Dflag5 -Dflag1 @rsp6.rsp");
+}
+
 } // end namespace tooling
 } // end namespace clang
Index: clang/lib/Tooling/JSONCompilationDatabase.cpp
===
--- clang/lib/Tooling/JSONCompilationDatabase.cpp
+++ clang/lib/Tooling/JSONCompilationDatabase.cpp
@@ -167,8 +167,8 @@
 llvm::sys::path::append(JSONDatabasePath, "compile_commands.json");
 auto Base = JSONCompilationDatabase::loadFromFile(
 JSONDatabasePath, ErrorMessage, JSONCommandLineSyntax::AutoDetect);
-return Base ? inferTargetAndDriverMode(
-  inferMissingCompileCommands(std::move(Base)))
+return Base ? inferTargetAndDriverMode(inferMissingCompileCommands(
+  expandResponseFiles(std::move(Base
 : nullptr;
   }
 };
Index: clang/lib/Tooling/ExpandResponseFilesCompilationDatabase.cpp
===
--- /dev/null
+++ clang/lib/Tooling/ExpandResponseFilesCompilationDatabase.cpp
@@ -0,0 +1,192 @@
+//===- ExpandResponseFileCompilationDataBase.cpp --===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "clang/Tooling/CompilationDatabase.h"
+#include "llvm/ADT/Triple.h"
+#include "llvm/Support/CommandLine.h"
+#include "llvm/Support/ConvertUTF.h"
+#include "llvm/Support/ErrorOr.h"
+#include 

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

[liu hui via Phabricator via cfe-commits]

lh123 updated this revision to Diff 230752.
lh123 added a comment.
Herald added a subscriber: mgorny.

Address comment


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

https://reviews.llvm.org/D70222

Files:
  clang/include/clang/Tooling/CompilationDatabase.h
  clang/lib/Tooling/CMakeLists.txt
  clang/lib/Tooling/CompilationDatabase.cpp
  clang/lib/Tooling/ExpandResponseFilesCompilationDatabase.cpp
  clang/lib/Tooling/JSONCompilationDatabase.cpp
  clang/unittests/Tooling/CompilationDatabaseTest.cpp

Index: clang/unittests/Tooling/CompilationDatabaseTest.cpp
===
--- clang/unittests/Tooling/CompilationDatabaseTest.cpp
+++ clang/unittests/Tooling/CompilationDatabaseTest.cpp
@@ -17,6 +17,7 @@
 #include "llvm/Support/TargetSelect.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
+#include 
 
 namespace clang {
 namespace tooling {
@@ -859,5 +860,76 @@
 "clang++ --driver-mode=g++ bar.cpp -D bar.cpp");
 }
 
+class ExpandResponseFilesTest : public MemDBTest {
+protected:
+  void SetUp() override {
+InnerDir = path(StringRef("inner"));
+std::error_code EC = llvm::sys::fs::create_directory(InnerDir);
+EXPECT_TRUE(!EC);
+
+llvm::sys::path::append(RspFileName1, InnerDir, "rsp1.rsp");
+std::ofstream RspFile1(RspFileName1.c_str());
+RspFile1 << "-Dflag1";
+RspFile1.close();
+
+RspFileName2 = path(StringRef("rsp2.rsp"));
+std::ofstream RspFile2(RspFileName2.c_str());
+RspFile2 << "-Dflag2 @rsp3.rsp";
+RspFile2.close();
+
+RspFileName3 = path(StringRef("rsp3.rsp"));
+std::ofstream RspFile3(RspFileName3.c_str());
+RspFile3 << "-Dflag3";
+RspFile3.close();
+
+RspFileName4 = path(StringRef("rsp4.rsp"));
+std::ofstream  RspFile4(RspFileName4.c_str());
+RspFile4 << "-Dflag4 @rsp4.rsp";
+RspFile4.close();
+
+llvm::sys::path::append(RspFileName5, InnerDir, "rsp5.rsp");
+std::ofstream RspFile5(RspFileName5.c_str());
+RspFile5 << "-Dflag5 @inner/rsp1.rsp";
+RspFile5.close();
+  }
+
+  void TearDown() override {
+llvm::sys::fs::remove(RspFileName5);
+llvm::sys::fs::remove(RspFileName4);
+llvm::sys::fs::remove(RspFileName3);
+llvm::sys::fs::remove(RspFileName2);
+llvm::sys::fs::remove(RspFileName1);
+llvm::sys::fs::remove(InnerDir);
+  }
+
+  std::string getCommand(llvm::StringRef F) {
+auto Results = expandResponseFiles(std::make_unique(Entries))
+   ->getCompileCommands(path(F));
+if (Results.empty()) {
+  return "none";
+}
+return llvm::join(Results[0].CommandLine, " ");
+  }
+
+  SmallString<128> InnerDir;
+  SmallString<128> RspFileName1;
+  SmallString<128> RspFileName2;
+  SmallString<128> RspFileName3;
+  SmallString<128> RspFileName4;
+  SmallString<128> RspFileName5;
+};
+
+TEST_F(ExpandResponseFilesTest, ExpandResponseFiles) {
+  // clang-format off
+  add("foo.cpp", "clang",
+  ("@inner/rsp1.rsp @rsp2.rsp @rsp4.rsp "
+"@" + RspFileName1 + " @inner/rsp5.rsp @rsp6.rsp")
+  .str());
+  // clang-format on
+  EXPECT_THAT(getCommand("foo.cpp"), "clang foo.cpp -D foo.cpp -Dflag1 -Dflag2 "
+ "-Dflag3 -Dflag4 @rsp4.rsp -Dflag1 "
+ "-Dflag5 -Dflag1 @rsp6.rsp");
+}
+
 } // end namespace tooling
 } // end namespace clang
Index: clang/lib/Tooling/JSONCompilationDatabase.cpp
===
--- clang/lib/Tooling/JSONCompilationDatabase.cpp
+++ clang/lib/Tooling/JSONCompilationDatabase.cpp
@@ -167,8 +167,8 @@
 llvm::sys::path::append(JSONDatabasePath, "compile_commands.json");
 auto Base = JSONCompilationDatabase::loadFromFile(
 JSONDatabasePath, ErrorMessage, JSONCommandLineSyntax::AutoDetect);
-return Base ? inferTargetAndDriverMode(
-  inferMissingCompileCommands(std::move(Base)))
+return Base ? expandResponseFiles(inferTargetAndDriverMode(
+  inferMissingCompileCommands(std::move(Base
 : nullptr;
   }
 };
Index: clang/lib/Tooling/ExpandResponseFilesCompilationDatabase.cpp
===
--- /dev/null
+++ clang/lib/Tooling/ExpandResponseFilesCompilationDatabase.cpp
@@ -0,0 +1,192 @@
+//===- ExpandResponseFileCompilationDataBase.cpp --===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "clang/Tooling/CompilationDatabase.h"
+#include "llvm/ADT/Triple.h"
+#include "llvm/Support/CommandLine.h"
+#include "llvm/Support/ConvertUTF.h"
+#include "llvm/Support/ErrorOr.h"
+#include "llvm/Support/MemoryBuffer.h"
+#include 

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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

Sorry wrote comments but forgot to hit submit :(




Comment at: clang/include/clang/Tooling/CompilationDatabase.h:229
+/// \return true if all @files were expanded successfully or there were none.
+bool expandResponseFiles(tooling::CompileCommand ,
+ llvm::cl::TokenizerCallback Tokenizer);

instead of exposing this functionality and handling them in Fixed and JSON 
compilation databases separately,  I was rather talking about creating a new 
type of compilation database.

as `inferTargetAndDriverMode` or `inferMissingCompileCommands` functions does. 
which will take an `Inner`/`Base` compilation database, and in its 
`getCompileCommands` method will try to expand any rsp files.

does that make sense?

Also please implement it in a new file. (but still put the declaration into 
this header)


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

https://reviews.llvm.org/D70222



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[liu hui via Phabricator via cfe-commits]

lh123 updated this revision to Diff 230060.
lh123 added a comment.

fixes some  bug and add more test.


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

https://reviews.llvm.org/D70222

Files:
  clang/include/clang/Tooling/CompilationDatabase.h
  clang/lib/Tooling/CompilationDatabase.cpp
  clang/lib/Tooling/JSONCompilationDatabase.cpp
  clang/unittests/Tooling/CompilationDatabaseTest.cpp

Index: clang/unittests/Tooling/CompilationDatabaseTest.cpp
===
--- clang/unittests/Tooling/CompilationDatabaseTest.cpp
+++ clang/unittests/Tooling/CompilationDatabaseTest.cpp
@@ -17,6 +17,7 @@
 #include "llvm/Support/TargetSelect.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
+#include 
 
 namespace clang {
 namespace tooling {
@@ -859,5 +860,132 @@
 "clang++ --driver-mode=g++ bar.cpp -D bar.cpp");
 }
 
+TEST(ExpandResponseFileTest, JSONCompilationDatabase) {
+  SmallString<128> TestDir;
+  std::error_code EC =
+  llvm::sys::fs::createUniqueDirectory("unittest", TestDir);
+  EXPECT_TRUE(!EC);
+
+  SmallString<128> TestMainFileName;
+  llvm::sys::path::append(TestMainFileName, TestDir, "main.cpp");
+
+  SmallString<128> InnerDir;
+  llvm::sys::path::append(InnerDir, TestDir, "inner");
+  EC = llvm::sys::fs::create_directory(InnerDir);
+  EXPECT_TRUE(!EC);
+
+  SmallString<128> RspFileName1;
+  llvm::sys::path::append(RspFileName1, InnerDir, "rsp1.rsp");
+  std::ofstream RspFile1(RspFileName1.c_str());
+  RspFile1 << "-Dflag1";
+  RspFile1.close();
+
+  SmallString<128> RspFileName2;
+  llvm::sys::path::append(RspFileName2, TestDir, "rsp2.rsp");
+  std::ofstream RspFile2(RspFileName2.c_str());
+  RspFile2 << "-Dflag2 @rsp3.rsp";
+  RspFile2.close();
+
+  SmallString<128> RspFileName3;
+  llvm::sys::path::append(RspFileName3, TestDir, "rsp3.rsp");
+  std::ofstream RspFile3(RspFileName3.c_str());
+  RspFile3 << "-Dflag3";
+  RspFile3.close();
+
+  SmallString<128> RspFileName4;
+  llvm::sys::path::append(RspFileName4, TestDir, "rsp4.rsp");
+  std::ofstream RspFile4(RspFileName4.c_str());
+  RspFile4 << "-Dflag4 @rsp4.rsp";
+  RspFile4.close();
+
+  SmallString<128> RspFileName5;
+  llvm::sys::path::append(RspFileName5, InnerDir, "rsp5.rsp");
+  std::ofstream RspFile5(RspFileName5.c_str());
+  RspFile5 << "-Dflag5 @inner/rsp1.rsp";
+  RspFile5.close();
+
+  SmallString<128> CompileCommandsFileName;
+  llvm::sys::path::append(CompileCommandsFileName, TestDir,
+  "compile_commands.json");
+  std::ofstream CompileCommandsFile(CompileCommandsFileName.c_str());
+  // clang-format off
+  CompileCommandsFile
+  << ("[{\"directory\": \"" + TestDir + "\","
+  "\"command\": \"clang @inner/rsp1.rsp @rsp2.rsp @rsp4.rsp "
+  "@" + RspFileName1 + " @inner/rsp5.rsp @rsp6.rsp\","
+  "\"file\": \"" + TestMainFileName + "\"}]").str();
+  // clang-format on
+  CompileCommandsFile.close();
+
+  std::string ErrorMessage;
+  auto JsonDatabase =
+  JSONCompilationDatabase::loadFromDirectory(TestDir, ErrorMessage);
+
+  EXPECT_TRUE(JsonDatabase);
+  auto FoundCommand = JsonDatabase->getCompileCommands(TestMainFileName);
+
+  EXPECT_TRUE(FoundCommand.size() == 1u) << ErrorMessage;
+  EXPECT_EQ(FoundCommand[0].Directory, TestDir) << ErrorMessage;
+  EXPECT_THAT(FoundCommand[0].CommandLine,
+  ElementsAre("clang", "-Dflag1", "-Dflag2", "-Dflag3", "-Dflag4",
+  "@rsp4.rsp", "-Dflag1", "-Dflag5", "-Dflag1",
+  "@rsp6.rsp"))
+  << ErrorMessage;
+  EXPECT_EQ(FoundCommand[0].Filename, TestMainFileName) << ErrorMessage;
+
+  llvm::sys::fs::remove(RspFileName5);
+  llvm::sys::fs::remove(RspFileName4);
+  llvm::sys::fs::remove(RspFileName3);
+  llvm::sys::fs::remove(RspFileName2);
+  llvm::sys::fs::remove(RspFileName1);
+  llvm::sys::fs::remove(InnerDir);
+  llvm::sys::fs::remove(TestDir);
+}
+
+TEST(ExpandResponseFileTest, FixedCompilationDatabase) {
+  SmallString<128> TestDir;
+  std::error_code EC =
+  llvm::sys::fs::createUniqueDirectory("unittest", TestDir);
+  EXPECT_TRUE(!EC);
+
+  SmallString<128> TestMainFileName;
+  llvm::sys::path::append(TestMainFileName, TestDir, "main.cpp");
+
+  SmallString<128> RspFileName1;
+  llvm::sys::path::append(RspFileName1, TestDir, "rsp1.rsp");
+  std::ofstream RspFile1(RspFileName1.c_str());
+  RspFile1 << "-Dflag1 @rsp2.rsp";
+  RspFile1.close();
+
+  SmallString<128> RspFileName2;
+  llvm::sys::path::append(RspFileName2, TestDir, "rsp2.rsp");
+  std::ofstream RspFile2(RspFileName2.c_str());
+  RspFile2 << "-Dflag2";
+  RspFile2.close();
+
+  SmallString<128> FixedCompilationFileName;
+  llvm::sys::path::append(FixedCompilationFileName, TestDir,
+  "compile_flags.txt");
+  std::ofstream CompileCommandsFile(FixedCompilationFileName.c_str());
+  CompileCommandsFile << "@rsp1.rsp";
+  CompileCommandsFile.close();
+  std::string ErrorMessage;
+  auto FixedDatabase =
+

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

[liu hui via Phabricator via cfe-commits]

lh123 updated this revision to Diff 230033.
lh123 added a comment.

fix typo in function document.


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

https://reviews.llvm.org/D70222

Files:
  clang/include/clang/Tooling/CompilationDatabase.h
  clang/lib/Tooling/CompilationDatabase.cpp
  clang/lib/Tooling/JSONCompilationDatabase.cpp
  clang/unittests/Tooling/CompilationDatabaseTest.cpp

Index: clang/unittests/Tooling/CompilationDatabaseTest.cpp
===
--- clang/unittests/Tooling/CompilationDatabaseTest.cpp
+++ clang/unittests/Tooling/CompilationDatabaseTest.cpp
@@ -17,6 +17,7 @@
 #include "llvm/Support/TargetSelect.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
+#include 
 
 namespace clang {
 namespace tooling {
@@ -859,5 +860,98 @@
 "clang++ --driver-mode=g++ bar.cpp -D bar.cpp");
 }
 
+TEST(ExpandResponseFileTest, JSONCompilationDatabase) {
+  SmallString<128> TestDir;
+  std::error_code EC = llvm::sys::fs::createUniqueDirectory("unittest", TestDir);
+  EXPECT_TRUE(!EC);
+
+  SmallString<128> TestMainFileName;
+  llvm::sys::path::append(TestMainFileName, TestDir, "main.cpp");
+
+  SmallString<128> FooFileName;
+  llvm::sys::path::append(FooFileName, TestDir, "foo.rsp");
+  std::ofstream FooFile(FooFileName.c_str());
+  FooFile << "-DFOO @bar.rsp";
+  FooFile.close();
+
+  SmallString<128> BarFileName;
+  llvm::sys::path::append(BarFileName, TestDir, "bar.rsp");
+  std::ofstream BarFile(BarFileName.c_str());
+  BarFile << "-DBAR";
+  BarFile.close();
+
+  SmallString<128> CompileCommandsFileName;
+  llvm::sys::path::append(CompileCommandsFileName, TestDir,
+  "compile_commands.json");
+  std::ofstream CompileCommandsFile(CompileCommandsFileName.c_str());
+  // clang-format off
+  CompileCommandsFile << ("[{\"directory\": \"" + TestDir + "\","
+  "\"command\": \"clang @foo.rsp\","
+  "\"file\": \"" + TestMainFileName + "\"}]").str();
+  // clang-format on
+  CompileCommandsFile.close();
+  std::string ErrorMessage;
+  auto JsonDatabase =
+  JSONCompilationDatabase::loadFromDirectory(TestDir, ErrorMessage);
+
+  EXPECT_TRUE(JsonDatabase);
+  auto FoundCommand = JsonDatabase->getCompileCommands(TestMainFileName);
+
+  EXPECT_TRUE(FoundCommand.size() == 1u) << ErrorMessage;
+  EXPECT_EQ(FoundCommand[0].Directory, TestDir) << ErrorMessage;
+  EXPECT_THAT(FoundCommand[0].CommandLine,
+  ElementsAre("clang", "-DFOO", "-DBAR"))
+  << ErrorMessage;
+  EXPECT_EQ(FoundCommand[0].Filename, TestMainFileName) << ErrorMessage;
+
+  llvm::sys::fs::remove(BarFileName);
+  llvm::sys::fs::remove(FooFileName);
+  llvm::sys::fs::remove(TestDir);
+}
+
+TEST(ExpandResponseFileTest, FixedCompilationDatabase) {
+  SmallString<128> TestDir;
+  std::error_code EC = llvm::sys::fs::createUniqueDirectory("unittest", TestDir);
+  EXPECT_TRUE(!EC);
+
+  SmallString<128> TestMainFileName;
+  llvm::sys::path::append(TestMainFileName, TestDir, "main.cpp");
+
+  SmallString<128> FooFileName;
+  llvm::sys::path::append(FooFileName, TestDir, "foo.rsp");
+  std::ofstream FooFile(FooFileName.c_str());
+  FooFile << "-DFOO @bar.rsp";
+  FooFile.close();
+
+  SmallString<128> BarFileName;
+  llvm::sys::path::append(BarFileName, TestDir, "bar.rsp");
+  std::ofstream BarFile(BarFileName.c_str());
+  BarFile << "-DBAR";
+  BarFile.close();
+
+  SmallString<128> FixedCompilationFileName;
+  llvm::sys::path::append(FixedCompilationFileName, TestDir,
+  "compile_flags.txt");
+  std::ofstream CompileCommandsFile(FixedCompilationFileName.c_str());
+  CompileCommandsFile << "@foo.rsp";
+  CompileCommandsFile.close();
+  std::string ErrorMessage;
+  auto FixedDatabase =
+  FixedCompilationDatabase::loadFromDirectory(TestDir, ErrorMessage);
+
+  EXPECT_TRUE(FixedDatabase);
+  auto FoundCommand = FixedDatabase->getCompileCommands(TestMainFileName);
+
+  EXPECT_TRUE(FoundCommand.size() == 1u) << ErrorMessage;
+  EXPECT_THAT(FoundCommand[0].CommandLine,
+  ElementsAre(EndsWith("clang-tool"), "-DFOO", "-DBAR",
+  EndsWith("main.cpp")))
+  << ErrorMessage;
+
+  llvm::sys::fs::remove(BarFileName);
+  llvm::sys::fs::remove(FooFileName);
+  llvm::sys::fs::remove(TestDir);
+}
+
 } // end namespace tooling
 } // end namespace clang
Index: clang/lib/Tooling/JSONCompilationDatabase.cpp
===
--- clang/lib/Tooling/JSONCompilationDatabase.cpp
+++ clang/lib/Tooling/JSONCompilationDatabase.cpp
@@ -313,16 +313,28 @@
 void JSONCompilationDatabase::getCommands(
 ArrayRef CommandsRef,
 std::vector ) const {
+  auto GetTokenizer = [](JSONCommandLineSyntax Syntax) {
+if (Syntax == JSONCommandLineSyntax::AutoDetect) {
+  Syntax = llvm::Triple(llvm::sys::getProcessTriple()).isOSWindows()
+   ? JSONCommandLineSyntax::Windows
+  

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

[liu hui via Phabricator via cfe-commits]

lh123 updated this revision to Diff 230031.
lh123 edited the summary of this revision.
lh123 added a comment.

Address comment


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

https://reviews.llvm.org/D70222

Files:
  clang/include/clang/Tooling/CompilationDatabase.h
  clang/lib/Tooling/CompilationDatabase.cpp
  clang/lib/Tooling/JSONCompilationDatabase.cpp
  clang/unittests/Tooling/CompilationDatabaseTest.cpp

Index: clang/unittests/Tooling/CompilationDatabaseTest.cpp
===
--- clang/unittests/Tooling/CompilationDatabaseTest.cpp
+++ clang/unittests/Tooling/CompilationDatabaseTest.cpp
@@ -17,6 +17,7 @@
 #include "llvm/Support/TargetSelect.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
+#include 
 
 namespace clang {
 namespace tooling {
@@ -859,5 +860,98 @@
 "clang++ --driver-mode=g++ bar.cpp -D bar.cpp");
 }
 
+TEST(ExpandResponseFileTest, JSONCompilationDatabase) {
+  SmallString<128> TestDir;
+  std::error_code EC = llvm::sys::fs::createUniqueDirectory("unittest", TestDir);
+  EXPECT_TRUE(!EC);
+
+  SmallString<128> TestMainFileName;
+  llvm::sys::path::append(TestMainFileName, TestDir, "main.cpp");
+
+  SmallString<128> FooFileName;
+  llvm::sys::path::append(FooFileName, TestDir, "foo.rsp");
+  std::ofstream FooFile(FooFileName.c_str());
+  FooFile << "-DFOO @bar.rsp";
+  FooFile.close();
+
+  SmallString<128> BarFileName;
+  llvm::sys::path::append(BarFileName, TestDir, "bar.rsp");
+  std::ofstream BarFile(BarFileName.c_str());
+  BarFile << "-DBAR";
+  BarFile.close();
+
+  SmallString<128> CompileCommandsFileName;
+  llvm::sys::path::append(CompileCommandsFileName, TestDir,
+  "compile_commands.json");
+  std::ofstream CompileCommandsFile(CompileCommandsFileName.c_str());
+  // clang-format off
+  CompileCommandsFile << ("[{\"directory\": \"" + TestDir + "\","
+  "\"command\": \"clang @foo.rsp\","
+  "\"file\": \"" + TestMainFileName + "\"}]").str();
+  // clang-format on
+  CompileCommandsFile.close();
+  std::string ErrorMessage;
+  auto JsonDatabase =
+  JSONCompilationDatabase::loadFromDirectory(TestDir, ErrorMessage);
+
+  EXPECT_TRUE(JsonDatabase);
+  auto FoundCommand = JsonDatabase->getCompileCommands(TestMainFileName);
+
+  EXPECT_TRUE(FoundCommand.size() == 1u) << ErrorMessage;
+  EXPECT_EQ(FoundCommand[0].Directory, TestDir) << ErrorMessage;
+  EXPECT_THAT(FoundCommand[0].CommandLine,
+  ElementsAre("clang", "-DFOO", "-DBAR"))
+  << ErrorMessage;
+  EXPECT_EQ(FoundCommand[0].Filename, TestMainFileName) << ErrorMessage;
+
+  llvm::sys::fs::remove(BarFileName);
+  llvm::sys::fs::remove(FooFileName);
+  llvm::sys::fs::remove(TestDir);
+}
+
+TEST(ExpandResponseFileTest, FixedCompilationDatabase) {
+  SmallString<128> TestDir;
+  std::error_code EC = llvm::sys::fs::createUniqueDirectory("unittest", TestDir);
+  EXPECT_TRUE(!EC);
+
+  SmallString<128> TestMainFileName;
+  llvm::sys::path::append(TestMainFileName, TestDir, "main.cpp");
+
+  SmallString<128> FooFileName;
+  llvm::sys::path::append(FooFileName, TestDir, "foo.rsp");
+  std::ofstream FooFile(FooFileName.c_str());
+  FooFile << "-DFOO @bar.rsp";
+  FooFile.close();
+
+  SmallString<128> BarFileName;
+  llvm::sys::path::append(BarFileName, TestDir, "bar.rsp");
+  std::ofstream BarFile(BarFileName.c_str());
+  BarFile << "-DBAR";
+  BarFile.close();
+
+  SmallString<128> FixedCompilationFileName;
+  llvm::sys::path::append(FixedCompilationFileName, TestDir,
+  "compile_flags.txt");
+  std::ofstream CompileCommandsFile(FixedCompilationFileName.c_str());
+  CompileCommandsFile << "@foo.rsp";
+  CompileCommandsFile.close();
+  std::string ErrorMessage;
+  auto FixedDatabase =
+  FixedCompilationDatabase::loadFromDirectory(TestDir, ErrorMessage);
+
+  EXPECT_TRUE(FixedDatabase);
+  auto FoundCommand = FixedDatabase->getCompileCommands(TestMainFileName);
+
+  EXPECT_TRUE(FoundCommand.size() == 1u) << ErrorMessage;
+  EXPECT_THAT(FoundCommand[0].CommandLine,
+  ElementsAre(EndsWith("clang-tool"), "-DFOO", "-DBAR",
+  EndsWith("main.cpp")))
+  << ErrorMessage;
+
+  llvm::sys::fs::remove(BarFileName);
+  llvm::sys::fs::remove(FooFileName);
+  llvm::sys::fs::remove(TestDir);
+}
+
 } // end namespace tooling
 } // end namespace clang
Index: clang/lib/Tooling/JSONCompilationDatabase.cpp
===
--- clang/lib/Tooling/JSONCompilationDatabase.cpp
+++ clang/lib/Tooling/JSONCompilationDatabase.cpp
@@ -313,16 +313,28 @@
 void JSONCompilationDatabase::getCommands(
 ArrayRef CommandsRef,
 std::vector ) const {
+  auto GetTokenizer = [](JSONCommandLineSyntax Syntax) {
+if (Syntax == JSONCommandLineSyntax::AutoDetect) {
+  Syntax = llvm::Triple(llvm::sys::getProcessTriple()).isOSWindows()
+   ? 

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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

Thanks, layering seems better now. But as Sam mentioned in the issue it would 
be better to have something like `expandRSPFiles` similar to 
`inferMissingCompileCommands`.
As the problem is not in the `JSONCompilationDatabase` itself, it can also 
occur in `FixedCompilationDatabase` or any other models clang might've in the 
future.

The CompilationDatabase returned by `expandRSPFiles` could wrap any other `CDB` 
and upon querying of commands, it can expand any RSP files returned by the 
underlying CDB.

This will also ensure minimum changes to the existing code, reducing any 
chances of breakage. As for unittests, please see 
`clang/unittests/Tooling/CompilationDatabaseTest.cpp`

and a small nit, when modifying existing files, please only run formatting on 
the lines you touched to preserve history.


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

https://reviews.llvm.org/D70222



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[liu hui via Phabricator via cfe-commits]

lh123 updated this revision to Diff 229812.
lh123 added a comment.

Respect `JSONCommandLineSyntax`


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

https://reviews.llvm.org/D70222

Files:
  clang/lib/Tooling/JSONCompilationDatabase.cpp

Index: clang/lib/Tooling/JSONCompilationDatabase.cpp
===
--- clang/lib/Tooling/JSONCompilationDatabase.cpp
+++ clang/lib/Tooling/JSONCompilationDatabase.cpp
@@ -24,6 +24,7 @@
 #include "llvm/Support/Allocator.h"
 #include "llvm/Support/Casting.h"
 #include "llvm/Support/CommandLine.h"
+#include "llvm/Support/ConvertUTF.h"
 #include "llvm/Support/ErrorOr.h"
 #include "llvm/Support/Host.h"
 #include "llvm/Support/MemoryBuffer.h"
@@ -49,9 +50,9 @@
 /// Assumes \-escaping for quoted arguments (see the documentation of
 /// unescapeCommandLine(...)).
 class CommandLineArgumentParser {
- public:
+public:
   CommandLineArgumentParser(StringRef CommandLine)
-  : Input(CommandLine), Position(Input.begin()-1) {}
+  : Input(CommandLine), Position(Input.begin() - 1) {}
 
   std::vector parse() {
 bool HasMoreInput = true;
@@ -63,46 +64,56 @@
 return CommandLine;
   }
 
- private:
+private:
   // All private methods return true if there is more input available.
 
   bool parseStringInto(std::string ) {
 do {
   if (*Position == '"') {
-if (!parseDoubleQuotedStringInto(String)) return false;
+if (!parseDoubleQuotedStringInto(String))
+  return false;
   } else if (*Position == '\'') {
-if (!parseSingleQuotedStringInto(String)) return false;
+if (!parseSingleQuotedStringInto(String))
+  return false;
   } else {
-if (!parseFreeStringInto(String)) return false;
+if (!parseFreeStringInto(String))
+  return false;
   }
 } while (*Position != ' ');
 return true;
   }
 
   bool parseDoubleQuotedStringInto(std::string ) {
-if (!next()) return false;
+if (!next())
+  return false;
 while (*Position != '"') {
-  if (!skipEscapeCharacter()) return false;
+  if (!skipEscapeCharacter())
+return false;
   String.push_back(*Position);
-  if (!next()) return false;
+  if (!next())
+return false;
 }
 return next();
   }
 
   bool parseSingleQuotedStringInto(std::string ) {
-if (!next()) return false;
+if (!next())
+  return false;
 while (*Position != '\'') {
   String.push_back(*Position);
-  if (!next()) return false;
+  if (!next())
+return false;
 }
 return next();
   }
 
   bool parseFreeStringInto(std::string ) {
 do {
-  if (!skipEscapeCharacter()) return false;
+  if (!skipEscapeCharacter())
+return false;
   String.push_back(*Position);
-  if (!next()) return false;
+  if (!next())
+return false;
 } while (*Position != ' ' && *Position != '"' && *Position != '\'');
 return true;
   }
@@ -116,7 +127,8 @@
 
   bool nextNonWhitespace() {
 do {
-  if (!next()) return false;
+  if (!next())
+return false;
 } while (*Position == ' ');
 return true;
   }
@@ -158,6 +170,124 @@
   return parser.parse();
 }
 
+bool expandResponseFile(llvm::StringRef FName, llvm::StringSaver ,
+llvm::cl::TokenizerCallback Tokenizer,
+SmallVectorImpl ) {
+  llvm::ErrorOr> MemBufOrErr =
+  llvm::MemoryBuffer::getFile(FName);
+  if (!MemBufOrErr)
+return false;
+  llvm::MemoryBuffer  = *MemBufOrErr.get();
+  StringRef Str(MemBuf.getBufferStart(), MemBuf.getBufferSize());
+
+  // If we have a UTF-16 byte order mark, convert to UTF-8 for parsing.
+  ArrayRef BufRef(MemBuf.getBufferStart(), MemBuf.getBufferEnd());
+  std::string UTF8Buf;
+  // It is called byte order marker but the UTF-8 BOM is actually not affected
+  // by the host system's endianness.
+  auto HasUtF8ByteOrderMark = [](ArrayRef S) {
+return (S.size() >= 3 && S[0] == '\xef' && S[1] == '\xbb' &&
+S[2] == '\xbf');
+  };
+  if (llvm::hasUTF16ByteOrderMark(BufRef)) {
+if (!convertUTF16ToUTF8String(BufRef, UTF8Buf))
+  return false;
+Str = StringRef(UTF8Buf);
+  }
+  // If we see UTF-8 BOM sequence at the beginning of a file, we shall remove
+  // these bytes before parsing.
+  // Reference: http://en.wikipedia.org/wiki/UTF-8#Byte_order_mark
+  else if (HasUtF8ByteOrderMark(BufRef))
+Str = StringRef(BufRef.data() + 3, BufRef.size() - 3);
+  // Tokenize the contents into NewArgv.
+  Tokenizer(Str, Saver, NewArgv, false);
+  return true;
+}
+
+bool expandResponseFiles(tooling::CompileCommand ,
+ llvm::cl::TokenizerCallback Tokenizer) {
+  bool AllExpanded = true;
+  struct ResponseFileRecord {
+llvm::StringRef File;
+size_t End;
+  };
+  std::vector  = Cmd.CommandLine;
+  // To detect recursive response files, we maintain a stack of files and the
+  // position of the last 

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

[liu hui via Phabricator via cfe-commits]

lh123 updated this revision to Diff 229807.
lh123 added a comment.

Move the implementation to `JSONCompilationDatabase`.


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

https://reviews.llvm.org/D70222

Files:
  clang/lib/Tooling/JSONCompilationDatabase.cpp

Index: clang/lib/Tooling/JSONCompilationDatabase.cpp
===
--- clang/lib/Tooling/JSONCompilationDatabase.cpp
+++ clang/lib/Tooling/JSONCompilationDatabase.cpp
@@ -24,6 +24,7 @@
 #include "llvm/Support/Allocator.h"
 #include "llvm/Support/Casting.h"
 #include "llvm/Support/CommandLine.h"
+#include "llvm/Support/ConvertUTF.h"
 #include "llvm/Support/ErrorOr.h"
 #include "llvm/Support/Host.h"
 #include "llvm/Support/MemoryBuffer.h"
@@ -49,9 +50,9 @@
 /// Assumes \-escaping for quoted arguments (see the documentation of
 /// unescapeCommandLine(...)).
 class CommandLineArgumentParser {
- public:
+public:
   CommandLineArgumentParser(StringRef CommandLine)
-  : Input(CommandLine), Position(Input.begin()-1) {}
+  : Input(CommandLine), Position(Input.begin() - 1) {}
 
   std::vector parse() {
 bool HasMoreInput = true;
@@ -63,46 +64,56 @@
 return CommandLine;
   }
 
- private:
+private:
   // All private methods return true if there is more input available.
 
   bool parseStringInto(std::string ) {
 do {
   if (*Position == '"') {
-if (!parseDoubleQuotedStringInto(String)) return false;
+if (!parseDoubleQuotedStringInto(String))
+  return false;
   } else if (*Position == '\'') {
-if (!parseSingleQuotedStringInto(String)) return false;
+if (!parseSingleQuotedStringInto(String))
+  return false;
   } else {
-if (!parseFreeStringInto(String)) return false;
+if (!parseFreeStringInto(String))
+  return false;
   }
 } while (*Position != ' ');
 return true;
   }
 
   bool parseDoubleQuotedStringInto(std::string ) {
-if (!next()) return false;
+if (!next())
+  return false;
 while (*Position != '"') {
-  if (!skipEscapeCharacter()) return false;
+  if (!skipEscapeCharacter())
+return false;
   String.push_back(*Position);
-  if (!next()) return false;
+  if (!next())
+return false;
 }
 return next();
   }
 
   bool parseSingleQuotedStringInto(std::string ) {
-if (!next()) return false;
+if (!next())
+  return false;
 while (*Position != '\'') {
   String.push_back(*Position);
-  if (!next()) return false;
+  if (!next())
+return false;
 }
 return next();
   }
 
   bool parseFreeStringInto(std::string ) {
 do {
-  if (!skipEscapeCharacter()) return false;
+  if (!skipEscapeCharacter())
+return false;
   String.push_back(*Position);
-  if (!next()) return false;
+  if (!next())
+return false;
 } while (*Position != ' ' && *Position != '"' && *Position != '\'');
 return true;
   }
@@ -116,7 +127,8 @@
 
   bool nextNonWhitespace() {
 do {
-  if (!next()) return false;
+  if (!next())
+return false;
 } while (*Position == ' ');
 return true;
   }
@@ -158,6 +170,124 @@
   return parser.parse();
 }
 
+bool expandResponseFile(llvm::StringRef FName, llvm::StringSaver ,
+llvm::cl::TokenizerCallback Tokenizer,
+SmallVectorImpl ) {
+  llvm::ErrorOr> MemBufOrErr =
+  llvm::MemoryBuffer::getFile(FName);
+  if (!MemBufOrErr)
+return false;
+  llvm::MemoryBuffer  = *MemBufOrErr.get();
+  StringRef Str(MemBuf.getBufferStart(), MemBuf.getBufferSize());
+
+  // If we have a UTF-16 byte order mark, convert to UTF-8 for parsing.
+  ArrayRef BufRef(MemBuf.getBufferStart(), MemBuf.getBufferEnd());
+  std::string UTF8Buf;
+  // It is called byte order marker but the UTF-8 BOM is actually not affected
+  // by the host system's endianness.
+  auto HasUtF8ByteOrderMark = [](ArrayRef S) {
+return (S.size() >= 3 && S[0] == '\xef' && S[1] == '\xbb' &&
+S[2] == '\xbf');
+  };
+  if (llvm::hasUTF16ByteOrderMark(BufRef)) {
+if (!convertUTF16ToUTF8String(BufRef, UTF8Buf))
+  return false;
+Str = StringRef(UTF8Buf);
+  }
+  // If we see UTF-8 BOM sequence at the beginning of a file, we shall remove
+  // these bytes before parsing.
+  // Reference: http://en.wikipedia.org/wiki/UTF-8#Byte_order_mark
+  else if (HasUtF8ByteOrderMark(BufRef))
+Str = StringRef(BufRef.data() + 3, BufRef.size() - 3);
+  // Tokenize the contents into NewArgv.
+  Tokenizer(Str, Saver, NewArgv, false);
+  return true;
+}
+
+bool expandResponseFiles(tooling::CompileCommand ,
+ llvm::cl::TokenizerCallback Tokenizer) {
+  bool AllExpanded = true;
+  struct ResponseFileRecord {
+llvm::StringRef File;
+size_t End;
+  };
+  std::vector  = Cmd.CommandLine;
+  // To detect recursive response files, we maintain a stack of files and the
+  // 

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

[liu hui via Phabricator via cfe-commits]

lh123 added a comment.

In D70222#1749533 , @kadircet wrote:

> Thanks for taking a look into this, the `rsp files` issue has came up before 
> in the past but there wasn't enough investment to implement it.
>
> Haven't checked the implementation in detail yet, I believe the layering 
> should be different;
>
> This is a common problem for all of the clang-related tools, as they all 
> share the same "compilation database" abstraction layer, therefore it would 
> be better to implement this at that layer so that other tools (e.g, 
> clang-tidy) can also benefit from this.
>  You can find the related code in 
> `clang/include/clang/Tooling/CompilationDatabase.h` and 
> `clang/lib/Tooling/CompilationDatabase.cpp`.
>
> Also compilation databases has been historically neglecting `Virtual File 
> System` abstractions, it is hard to change it now. But would be great if you 
> could try to keep that in mind while performing reads.
>
> So would you mind making such changes ?




In D70222#1749533 , @kadircet wrote:

> Thanks for taking a look into this, the `rsp files` issue has came up before 
> in the past but there wasn't enough investment to implement it.
>
> Haven't checked the implementation in detail yet, I believe the layering 
> should be different;
>
> This is a common problem for all of the clang-related tools, as they all 
> share the same "compilation database" abstraction layer, therefore it would 
> be better to implement this at that layer so that other tools (e.g, 
> clang-tidy) can also benefit from this.
>  You can find the related code in 
> `clang/include/clang/Tooling/CompilationDatabase.h` and 
> `clang/lib/Tooling/CompilationDatabase.cpp`.
>
> Also compilation databases has been historically neglecting `Virtual File 
> System` abstractions, it is hard to change it now. But would be great if you 
> could try to keep that in mind while performing reads.
>
> So would you mind making such changes ?


Ok, I will look into this.


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

https://reviews.llvm.org/D70222



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

Thanks for taking a look into this, the `rsp files` issue has came up before in 
the past but there wasn't enough investment to implement it.

Haven't checked the implementation in detail yet, I believe the layering should 
be different;

This is a common problem for all of the clang-related tools, as they all share 
the same "compilation database" abstraction layer, therefore it would be better 
to implement this at that layer so that other tools (e.g, clang-tidy) can also 
benefit from this.
You can find the related code in 
`clang/include/clang/Tooling/CompilationDatabase.h` and 
`clang/lib/Tooling/CompilationDatabase.cpp`.

Also compilation databases has been historically neglecting `Virtual File 
System` abstractions, it is hard to change it now. But would be great if you 
could try to keep that in mind while performing reads.

So would you mind making such changes ?


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

https://reviews.llvm.org/D70222



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[liu hui via Phabricator via cfe-commits]

lh123 updated this revision to Diff 229759.
lh123 added a comment.

format patch


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

https://reviews.llvm.org/D70222

Files:
  clang-tools-extra/clangd/GlobalCompilationDatabase.cpp

Index: clang-tools-extra/clangd/GlobalCompilationDatabase.cpp
===
--- clang-tools-extra/clangd/GlobalCompilationDatabase.cpp
+++ clang-tools-extra/clangd/GlobalCompilationDatabase.cpp
@@ -17,8 +17,10 @@
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SmallString.h"
+#include "llvm/Support/ConvertUTF.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/Path.h"
+#include "llvm/Support/StringSaver.h"
 #include 
 #include 
 #include 
@@ -27,6 +29,124 @@
 namespace clangd {
 namespace {
 
+bool expandResponseFile(llvm::StringRef FName, llvm::StringSaver ,
+llvm::cl::TokenizerCallback Tokenizer,
+SmallVectorImpl ) {
+  llvm::ErrorOr> MemBufOrErr =
+  llvm::MemoryBuffer::getFile(FName);
+  if (!MemBufOrErr)
+return false;
+  llvm::MemoryBuffer  = *MemBufOrErr.get();
+  StringRef Str(MemBuf.getBufferStart(), MemBuf.getBufferSize());
+
+  // If we have a UTF-16 byte order mark, convert to UTF-8 for parsing.
+  ArrayRef BufRef(MemBuf.getBufferStart(), MemBuf.getBufferEnd());
+  std::string UTF8Buf;
+  // It is called byte order marker but the UTF-8 BOM is actually not affected
+  // by the host system's endianness.
+  auto HasUtF8ByteOrderMark = [](ArrayRef S) {
+return (S.size() >= 3 && S[0] == '\xef' && S[1] == '\xbb' &&
+S[2] == '\xbf');
+  };
+  if (llvm::hasUTF16ByteOrderMark(BufRef)) {
+if (!convertUTF16ToUTF8String(BufRef, UTF8Buf))
+  return false;
+Str = StringRef(UTF8Buf);
+  }
+  // If we see UTF-8 BOM sequence at the beginning of a file, we shall remove
+  // these bytes before parsing.
+  // Reference: http://en.wikipedia.org/wiki/UTF-8#Byte_order_mark
+  else if (HasUtF8ByteOrderMark(BufRef))
+Str = StringRef(BufRef.data() + 3, BufRef.size() - 3);
+  // Tokenize the contents into NewArgv.
+  Tokenizer(Str, Saver, NewArgv, false);
+  return true;
+}
+
+bool expandResponseFiles(tooling::CompileCommand ,
+ llvm::cl::TokenizerCallback Tokenizer) {
+  bool AllExpanded = true;
+  struct ResponseFileRecord {
+llvm::StringRef File;
+size_t End;
+  };
+  std::vector  = Cmd.CommandLine;
+  // To detect recursive response files, we maintain a stack of files and the
+  // position of the last argument in the file. This position is updated
+  // dynamically as we recursively expand files.
+  SmallVector FileStack;
+
+  // Push a dummy entry that represents the initial command line, removing
+  // the need to check for an empty list.
+  FileStack.push_back({"", Argv.size()});
+
+  // Don't cache Argv.size() because it can change.
+  for (unsigned I = 0; I != Argv.size();) {
+while (I == FileStack.back().End) {
+  // Passing the end of a file's argument list, so we can remove it from the
+  // stack.
+  FileStack.pop_back();
+}
+
+std::string  = Argv[I];
+
+if (Arg[0] != '@') {
+  ++I;
+  continue;
+}
+SmallString<128> ResponseFile;
+if (llvm::sys::path::is_relative([1])) {
+  llvm::sys::path::append(ResponseFile, Cmd.Directory, [1]);
+}
+llvm::sys::path::remove_dots(ResponseFile);
+
+auto IsEquivalent = [ResponseFile](const ResponseFileRecord ) {
+  return llvm::sys::fs::equivalent(RFile.File, ResponseFile);
+};
+
+// Check for recursive response files.
+if (std::any_of(FileStack.begin() + 1, FileStack.end(), IsEquivalent)) {
+  // This file is recursive, so we leave it in the argument stream and
+  // move on.
+  AllExpanded = false;
+  ++I;
+  continue;
+}
+
+// Replace this response file argument with the tokenization of its
+// contents.  Nested response files are expanded in subsequent iterations.
+SmallVector ExpandedArgv;
+llvm::BumpPtrAllocator Alloc;
+llvm::StringSaver Saver(Alloc);
+llvm::SmallVector T;
+if (!expandResponseFile(ResponseFile, Saver, Tokenizer, ExpandedArgv)) {
+  // We couldn't read this file, so we leave it in the argument stream and
+  // move on.
+  AllExpanded = false;
+  ++I;
+  continue;
+}
+
+for (ResponseFileRecord  : FileStack) {
+  // Increase the end of all active records by the number of newly expanded
+  // arguments, minus the response file itself.
+  Record.End += ExpandedArgv.size() - 1;
+}
+
+FileStack.push_back({ResponseFile, I + ExpandedArgv.size()});
+Argv.erase(Argv.begin() + I);
+Argv.insert(Argv.begin() + I, ExpandedArgv.begin(), ExpandedArgv.end());
+  }
+
+  // If successful, the top of the file stack will mark the end of the Argv
+  // stream. A failure here indicates a bug in the stack popping logic above.
+ 

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

[liu hui via Phabricator via cfe-commits]

lh123 updated this revision to Diff 229758.
lh123 added a comment.

update diff


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

https://reviews.llvm.org/D70222

Files:
  clang-tools-extra/clangd/GlobalCompilationDatabase.cpp

Index: clang-tools-extra/clangd/GlobalCompilationDatabase.cpp
===
--- clang-tools-extra/clangd/GlobalCompilationDatabase.cpp
+++ clang-tools-extra/clangd/GlobalCompilationDatabase.cpp
@@ -17,8 +17,10 @@
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SmallString.h"
+#include "llvm/Support/ConvertUTF.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/Path.h"
+#include "llvm/Support/StringSaver.h"
 #include 
 #include 
 #include 
@@ -27,6 +29,124 @@
 namespace clangd {
 namespace {
 
+bool expandResponseFile(llvm::StringRef FName, llvm::StringSaver ,
+llvm::cl::TokenizerCallback Tokenizer,
+SmallVectorImpl ) {
+  llvm::ErrorOr> MemBufOrErr =
+  llvm::MemoryBuffer::getFile(FName);
+  if (!MemBufOrErr)
+return false;
+  llvm::MemoryBuffer  = *MemBufOrErr.get();
+  StringRef Str(MemBuf.getBufferStart(), MemBuf.getBufferSize());
+
+  // If we have a UTF-16 byte order mark, convert to UTF-8 for parsing.
+  ArrayRef BufRef(MemBuf.getBufferStart(), MemBuf.getBufferEnd());
+  std::string UTF8Buf;
+  // It is called byte order marker but the UTF-8 BOM is actually not affected
+  // by the host system's endianness.
+  auto HasUtF8ByteOrderMark = [](ArrayRef S) {
+return (S.size() >= 3 && S[0] == '\xef' && S[1] == '\xbb' &&
+S[2] == '\xbf');
+  };
+  if (llvm::hasUTF16ByteOrderMark(BufRef)) {
+if (!convertUTF16ToUTF8String(BufRef, UTF8Buf))
+  return false;
+Str = StringRef(UTF8Buf);
+  }
+  // If we see UTF-8 BOM sequence at the beginning of a file, we shall remove
+  // these bytes before parsing.
+  // Reference: http://en.wikipedia.org/wiki/UTF-8#Byte_order_mark
+  else if (HasUtF8ByteOrderMark(BufRef))
+Str = StringRef(BufRef.data() + 3, BufRef.size() - 3);
+  // Tokenize the contents into NewArgv.
+  Tokenizer(Str, Saver, NewArgv, false);
+  return true;
+}
+
+bool expandResponseFiles(tooling::CompileCommand ,
+ llvm::cl::TokenizerCallback Tokenizer) {
+  bool AllExpanded = true;
+  struct ResponseFileRecord {
+llvm::StringRef File;
+size_t End;
+  };
+  std::vector  = Cmd.CommandLine;
+  // To detect recursive response files, we maintain a stack of files and the
+  // position of the last argument in the file. This position is updated
+  // dynamically as we recursively expand files.
+  SmallVector FileStack;
+
+  // Push a dummy entry that represents the initial command line, removing
+  // the need to check for an empty list.
+  FileStack.push_back({"", Argv.size()});
+
+  // Don't cache Argv.size() because it can change.
+  for (unsigned I = 0; I != Argv.size();) {
+while (I == FileStack.back().End) {
+  // Passing the end of a file's argument list, so we can remove it from the
+  // stack.
+  FileStack.pop_back();
+}
+
+std::string  = Argv[I];
+
+if (Arg[0] != '@') {
+  ++I;
+  continue;
+}
+SmallString<128> ResponseFile;
+if (llvm::sys::path::is_relative([1])) {
+  llvm::sys::path::append(ResponseFile, Cmd.Directory, [1]);
+}
+llvm::sys::path::remove_dots(ResponseFile);
+
+auto IsEquivalent = [ResponseFile](const ResponseFileRecord ) {
+  return llvm::sys::fs::equivalent(RFile.File, ResponseFile);
+};
+
+// Check for recursive response files.
+if (std::any_of(FileStack.begin() + 1, FileStack.end(), IsEquivalent)) {
+  // This file is recursive, so we leave it in the argument stream and
+  // move on.
+  AllExpanded = false;
+  ++I;
+  continue;
+}
+
+// Replace this response file argument with the tokenization of its
+// contents.  Nested response files are expanded in subsequent iterations.
+SmallVector ExpandedArgv;
+llvm::BumpPtrAllocator Alloc;
+llvm::StringSaver Saver(Alloc);
+llvm::SmallVector T;
+if (!expandResponseFile(ResponseFile, Saver, Tokenizer, ExpandedArgv)) {
+  // We couldn't read this file, so we leave it in the argument stream and
+  // move on.
+  AllExpanded = false;
+  ++I;
+  continue;
+}
+
+for (ResponseFileRecord  : FileStack) {
+  // Increase the end of all active records by the number of newly expanded
+  // arguments, minus the response file itself.
+  Record.End += ExpandedArgv.size() - 1;
+}
+
+FileStack.push_back({ResponseFile, I + ExpandedArgv.size()});
+Argv.erase(Argv.begin() + I);
+Argv.insert(Argv.begin() + I, ExpandedArgv.begin(), ExpandedArgv.end());
+  }
+
+  // If successful, the top of the file stack will mark the end of the Argv
+  // stream. A failure here indicates a bug in the stack popping logic above.
+  

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

[liu hui via Phabricator via cfe-commits]

lh123 added a comment.

I don't know much about how to write unit tests, but it works fine on my 
computer.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70222



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[liu hui via Phabricator via cfe-commits]

lh123 created this revision.
lh123 added reviewers: sammccall, ilya-biryukov, hokein.
lh123 added a project: clang-tools-extra.
Herald added subscribers: cfe-commits, usaxena95, kadircet, arphaman, jkorous, 
MaskRay.
Herald added a project: clang.

Add support for .rsp files in compile_commands.json.

expand response file in `OverlayCDB`


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D70222

Files:
  clang-tools-extra/clangd/GlobalCompilationDatabase.cpp

Index: clang-tools-extra/clangd/GlobalCompilationDatabase.cpp
===
--- clang-tools-extra/clangd/GlobalCompilationDatabase.cpp
+++ clang-tools-extra/clangd/GlobalCompilationDatabase.cpp
@@ -17,8 +17,15 @@
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SmallString.h"
+#include "llvm/ADT/StringExtras.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/ADT/Triple.h"
+#include "llvm/Support/CommandLine.h"
+#include "llvm/Support/ConvertUTF.h"
 #include "llvm/Support/FileSystem.h"
+#include "llvm/Support/Host.h"
 #include "llvm/Support/Path.h"
+#include "llvm/Support/StringSaver.h"
 #include 
 #include 
 #include 
@@ -27,6 +34,123 @@
 namespace clangd {
 namespace {
 
+// It is called byte order marker but the UTF-8 BOM is actually not affected
+// by the host system's endianness.
+static bool hasUTF8ByteOrderMark(ArrayRef S) {
+  return (S.size() >= 3 && S[0] == '\xef' && S[1] == '\xbb' && S[2] == '\xbf');
+}
+
+static bool expandResponseFile(llvm::StringRef FName, llvm::StringSaver ,
+   llvm::cl::TokenizerCallback Tokenizer,
+
+   SmallVectorImpl ) {
+  llvm::ErrorOr> MemBufOrErr =
+  llvm::MemoryBuffer::getFile(FName);
+  if (!MemBufOrErr)
+return false;
+  llvm::MemoryBuffer  = *MemBufOrErr.get();
+  StringRef Str(MemBuf.getBufferStart(), MemBuf.getBufferSize());
+
+  // If we have a UTF-16 byte order mark, convert to UTF-8 for parsing.
+  ArrayRef BufRef(MemBuf.getBufferStart(), MemBuf.getBufferEnd());
+  std::string UTF8Buf;
+  if (llvm::hasUTF16ByteOrderMark(BufRef)) {
+if (!convertUTF16ToUTF8String(BufRef, UTF8Buf))
+  return false;
+Str = StringRef(UTF8Buf);
+  }
+  // If we see UTF-8 BOM sequence at the beginning of a file, we shall remove
+  // these bytes before parsing.
+  // Reference: http://en.wikipedia.org/wiki/UTF-8#Byte_order_mark
+  else if (hasUTF8ByteOrderMark(BufRef))
+Str = StringRef(BufRef.data() + 3, BufRef.size() - 3);
+  // Tokenize the contents into NewArgv.
+  Tokenizer(Str, Saver, NewArgv, false);
+  return true;
+}
+
+static bool expandResponseFiles(tooling::CompileCommand ,
+llvm::cl::TokenizerCallback Tokenizer) {
+  bool AllExpanded = true;
+  struct ResponseFileRecord {
+llvm::StringRef File;
+size_t End;
+  };
+  std::vector  = Cmd.CommandLine;
+  // To detect recursive response files, we maintain a stack of files and the
+  // position of the last argument in the file. This position is updated
+  // dynamically as we recursively expand files.
+  SmallVector FileStack;
+
+  // Push a dummy entry that represents the initial command line, removing
+  // the need to check for an empty list.
+  FileStack.push_back({"", Argv.size()});
+
+  // Don't cache Argv.size() because it can change.
+  for (unsigned I = 0; I != Argv.size();) {
+while (I == FileStack.back().End) {
+  // Passing the end of a file's argument list, so we can remove it from the
+  // stack.
+  FileStack.pop_back();
+}
+
+std::string  = Argv[I];
+
+if (Arg[0] != '@') {
+  ++I;
+  continue;
+}
+SmallString<128> ResponseFile;
+if (llvm::sys::path::is_relative([1])) {
+  llvm::sys::path::append(ResponseFile, Cmd.Directory, [1]);
+}
+auto IsEquivalent = [ResponseFile](const ResponseFileRecord ) {
+  return llvm::sys::fs::equivalent(RFile.File, ResponseFile);
+};
+
+// Check for recursive response files.
+if (std::any_of(FileStack.begin() + 1, FileStack.end(), IsEquivalent)) {
+  // This file is recursive, so we leave it in the argument stream and
+  // move on.
+  AllExpanded = false;
+  ++I;
+  continue;
+}
+
+// Replace this response file argument with the tokenization of its
+// contents.  Nested response files are expanded in subsequent iterations.
+SmallVector ExpandedArgv;
+llvm::BumpPtrAllocator Alloc;
+llvm::StringSaver Saver(Alloc);
+llvm::SmallVector T;
+if (!expandResponseFile(ResponseFile, Saver, Tokenizer, ExpandedArgv)) {
+  // We couldn't read this file, so we leave it in the argument stream and
+  // move on.
+  AllExpanded = false;
+  ++I;
+  continue;
+}
+
+for (ResponseFileRecord  : FileStack) {
+  // Increase the end of all active records by the number of newly expanded
+  // arguments, minus the response file itself.
+  Record.End +=