[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 &Cmd : 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<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
> > > > ```
> > > 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 &Saver, TokenizerCallback Tokenizer, SmallVectorImpl<const char *> &NewArgv, bool MarkEOLs, bool RelativeNames)` if we expose it in `CommandLine.h`
> `static bool ExpandResponseFile(StringRef FName, StringSaver &Saver, TokenizerCallback Tokenizer, SmallVectorImpl<const char *> &NewArgv, 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 &FS`, instead of duplicating the logic.
Namely changing the signature to:

```
bool ExpandResponseFiles(StringSaver &Saver, TokenizerCallback Tokenizer,
                         llvm::vfs::FileSystem &FS, SmallVectorImpl<const char *> &Argv,
                         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<CompilationDatabase> expandResponseFiles(std::unique_ptr<CompilationDatabase> Base)` to accept a `IntrusiveRefCntPtr<FileSystem> 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