[PATCH] D47266: Update thin-lto cache file atimes when on windows

[Peter Collingbourne via Phabricator via llvm-commits]

pcc added a comment.

You should be able to avoid adding the bool argument to `openNativeFileInternal` as well.



================
Comment at: include/llvm/Support/FileSystem.h:726
+
+  ///Force files Atime to be updated on access. Only makes a difference on windows.
+  OF_UpdateAtime = 16,
----------------
Nit: add space between `///` and the start of the comment.


================
Comment at: lib/LTO/Caching.cpp:36
     // First, see if we have a cache hit.
-    ErrorOr<std::unique_ptr<MemoryBuffer>> MBOrErr =
-        MemoryBuffer::getFile(EntryPath);
+    int FD;
+    SmallString<64> ResultPath;
----------------
I think this is leaking the FD; you need to close it after creating the map. Same in ThinLTOCodeGenerator.cpp.


================
Comment at: lib/Support/Windows/Path.inc:1083
+  if (ForceUpdateAtime)
+    Access |= FILE_WRITE_ATTRIBUTES;
+
----------------
Move this into `nativeAccess`.


================
Comment at: lib/Support/Windows/Path.inc:1133
     return errorCodeToError(EC);
   if (Flags & OF_Delete) {
     if ((EC = setDeleteDisposition(Result, true))) {
----------------
Move the call to `updateAccessTime` here (or better yet just inline the code).


https://reviews.llvm.org/D47266