kadircet added inline comments.
================
Comment at: clang/lib/Tooling/ExpandResponseFilesCompilationDatabase.cpp:28
+ llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> 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<CompileCommand> expand(std::vector<CompileCommand> Cmds) const {
+ llvm::ErrorOr<std::string> 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
