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



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to