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