[PATCH] D70857: [llvm][Support] Take in CurrentDirectory as a parameter in ExpandResponseFiles

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added inline comments.
This revision is now accepted and ready to land.


================
Comment at: llvm/include/llvm/Support/CommandLine.h:1972
+/// \param [in] CurrentDir Path used to resolve relative rsp files when \p
+/// RelativeNames is set to true, when set to None, process' cwd is used
+/// instead.
----------------
This seems confusing.
Suppose we have `@x/a`, which includes `@y/b`

Relative=true, CurrentDir=none: we read ./x/y/b
Relative=true, CurrentDir=foo: we read foo/x/y/b
Relative=false, CurrentDir=none: we read ./y/b
Relative=false, CurrentDir=foo: we read... ./y/b?

Seems like it would be clearer to just say relative paths are relative to this directory, so the last one would be foo/y/b.

Actually looking at the code, I think that's what you implemented?


================
Comment at: llvm/lib/Support/CommandLine.cpp:1105
+    SmallString<128> ResponseFile;
+    llvm::sys::path::append(ResponseFile, "@", BasePath, FileName);
+    Arg = Saver.save(ResponseFile.c_str()).data();
----------------
This is @/BasePath I think (args are segments).

(I think this means you were running the wrong tests)


================
Comment at: llvm/lib/Support/CommandLine.cpp:1183
             ExpandResponseFile(FName, Saver, Tokenizer, ExpandedArgv, MarkEOLs,
-                               RelativeNames, FS)) {
+                               RelativeNames, FS, CurrentDir)) {
       // We couldn't read this file, so we leave it in the argument stream and
----------------
this is subtle, consider a comment: CurrentDir is only relevant for "top-level" expansions || !RelativeNames, but nested ones always have absolute paths if RelativeNames so CurrentDir is ignored.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70857