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

[Zachary Turner via Phabricator via llvm-commits]

zturner added a comment.

4 minutes to 2 seconds is pretty impressive!



================
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)
----------------
pcc wrote:
> 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.
Eh, the style guide says "use auto where it makes the code more readable and easier to maintain".  Seems to fit the bill here.  That said, it's also minor and like you I don't feel strongly enough to push one way or the other.


================
Comment at: llvm/include/llvm/Support/FileSystem.h:169
 
-  file_status(file_type Type) : Type(Type) {}
+  basic_file_status(file_type Type) : Type(Type) {}
 
----------------
Should this be `explicit`?


================
Comment at: llvm/include/llvm/Support/FileSystem.h:235
+
+  file_status(file_type Type) : basic_file_status(Type) {}
+
----------------
Should this be marked `explicit`?


================
Comment at: llvm/include/llvm/Support/FileSystem.h:413
 ///          not.
-bool exists(file_status status);
+bool exists(basic_file_status status);
 
----------------
I just noticed all of these functions pass by value.  I can't see a good reason for that, so can you make them pass by const reference (especially since now you'll be slicing otherwise).


================
Comment at: llvm/lib/Support/CachePruning.cpp:186
   // Keep track of space
   std::set<std::pair<uint64_t, std::string>> FileSizes;
   uint64_t TotalSize = 0;
----------------
Unrelated, but I find it surprising this is using a `std::set`.  Wouldn't you get better performance by using a `StringMap<uint64_t>`? or a `DenseSet<std::pair<uint64_t, std::string>>`?


https://reviews.llvm.org/D38716