[PATCH] D38716: Support: Have directory_iterator::status() return FindFirstFileEx/FindNextFile results on Windows.

[Adrian McCarthy via Phabricator via llvm-commits]

amccarth added a comment.

Just a drive-by comment.



================
Comment at: clang-tools-extra/modularize/CoverageChecker.cpp:250
     std::string File(I->path());
-    I->status(Status);
-    sys::fs::file_type Type = Status.type();
+    llvm::ErrorOr<sys::fs::basic_file_status> Status = I->status();
+    if (!Status)
----------------
Just a thought...

Is there much value in explicitly declaring the return type, especially since it happens a lot in this patch?  I know the LLVM Coding Standards don't want us to always use auto, but, here, the type is so garrulous that it kind of obscures what's going on.  Consider:

```
const auto ErrorOrStatus = I->status();
if (!ErrorOrStatus)
  return false;
ss::fs::file_type Type = ErrorOrStatus->type();
```
Another thought is that `ErrorOr<>` might be overkill for methods that get the file or directory status.  Perhaps any error in retrieving the status should be represented in the status object itself.  The most likely error is probably that the file or directory doesn't exist.  Given that there's an `exists()` function that takes a status, it seems status needs to be able to represent some error conditions anyway.


https://reviews.llvm.org/D38716