[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: llvm/include/llvm/Support/FileSystem.h:169
 
-  file_status(file_type Type) : Type(Type) {}
+  basic_file_status(file_type Type) : Type(Type) {}
 
----------------
zturner wrote:
> Should this be `explicit`?
Makes sense, done.


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


================
Comment at: llvm/include/llvm/Support/FileSystem.h:413
 ///          not.
-bool exists(file_status status);
+bool exists(basic_file_status status);
 
----------------
zturner wrote:
> 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).
Done.


================
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;
----------------
zturner wrote:
> 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>>`?
I think we need to keep the set ordered by size for determinism (below we end up deleting in reverse set order, i.e. in decreasing order of size), and neither of those data structures will keep that ordering, I believe. I've added a comment here.


https://reviews.llvm.org/D38716