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

[Peter Collingbourne via Phabricator via llvm-commits]

pcc added inline comments.


================
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)
----------------
amccarth wrote:
> 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.
1. Yeah, it's a little ugly in this file, but in most of the rest of the code we have `using` declarations which mitigate this somewhat. I don't feel so strongly about it that I'd want to deviate from the style guide.

2. Maybe. The alternative solution, of course, would be to remove the ability to represent error conditions in `file_status`. It may be worth looking at which of those makes the code more readable, but it seems beyond the scope of this patch.


https://reviews.llvm.org/D38716