[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<CompilationDatabase> 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 &Cmd : 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<const char*, 20> Argvs;
Argvs.reserve(Cmd.CommandLine.size());
bool SeenRSPFile = false;
for(auto &Argv : Cmd.CommandLine) {
  Argvs.push_back(Argv.c_str());
  SeenRSPFile |= Argv.front() == '@';
}
if(!SeenRSPFile)
  continue;

// call ExpandResponseFiles with Argvs, convert back to vector<std::string> and assign to Cmd.CommandLine
```


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

https://reviews.llvm.org/D70222