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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits