[PATCH] D70769: [Support] add vfs support for ExpandResponseFiles

[Kadir Cetinkaya via Phabricator via llvm-commits]

kadircet added inline comments.


================
Comment at: llvm/include/llvm/Support/CommandLine.h:47
+class FileSystem;
+IntrusiveRefCntPtr<FileSystem> getRealFileSystem();
+
----------------
now that we are also pulling the the function, I think it is OK to include the header instead.


================
Comment at: llvm/lib/Support/CommandLine.cpp:1073
 
+  llvm::ErrorOr<std::string> CurrDir = FS.getCurrentWorkingDirectory();
+  if (!CurrDir)
----------------
you can move this to the beginning of the function, to exit immediately, without checking for anything else.


================
Comment at: llvm/lib/Support/CommandLine.cpp:1074
+  llvm::ErrorOr<std::string> CurrDir = FS.getCurrentWorkingDirectory();
+  if (!CurrDir)
+    return false;
----------------
it is sad that, ExpandResponseFile returns a bool, but that's a battle for another day.

unfortunately, it is not enough return false in this case you need to consume the error with `llvm::consumeError` before exiting the scope.


================
Comment at: llvm/lib/Support/CommandLine.cpp:1146
     const char *FName = Arg + 1;
-    auto IsEquivalent = [FName](const ResponseFileRecord &RFile) {
-      return sys::fs::equivalent(RFile.File, FName);
+    auto IsEquivalent = [FName, &FS](const ResponseFileRecord &RFile) {
+      SmallVector<char, 128> LHSPath;
----------------
there are `FileSystem::status` and `Status::equivalent` functions to implement `llvm::sys::fs:equivalent` behaviour


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70769