[PATCH] D111875: [Support] [Windows] Manually clean up temp files on network shares

[James Henderson via Phabricator via llvm-commits]

jhenderson accepted this revision.
jhenderson added a comment.
This revision is now accepted and ready to land.

Mostly minor comments. LGTM with them addressed (or not, if you disagree).



================
Comment at: llvm/lib/Support/Path.cpp:1209
 #ifdef _WIN32
-  // On windows closing will remove the file.
-  TmpName = "";
-  return Error::success();
+  // On windows closing will remove the file, if we managed to set
+  // the delete disposition. If not, remove it manually.
----------------
Whilst modifying the comment, let's capitalize windows -> Windows (since it's a proper noun). I also think it needs a comma.


================
Comment at: llvm/lib/Support/Path.cpp:1237
   auto H = reinterpret_cast<HANDLE>(_get_osfhandle(FD));
-  std::error_code RenameEC = setDeleteDisposition(H, false);
+  auto trySetDeleteDisposition = [this](HANDLE H, bool Intent) {
+    bool Delete = Intent;
----------------
I think the consensus I've seen elsewhere is that a lambda is a function object, i.e. a variable, not a function, so needs `UpperCamelCase` name styling.


================
Comment at: llvm/lib/Support/Path.cpp:1318
+  bool Delete = true;
+  // TODO: This causes duplicate calls to setDeleteDisposition as createUniqueFile
+  // also calls it internally, but we can't easily know whether it succeeded or not.
----------------
Nit: clang-format.


================
Comment at: llvm/lib/Support/Path.cpp:1326
+  }
+  // If we weren't able to set flag for implicitly deleting the file on
+  // close, make a note to delete it manually.
----------------



================
Comment at: llvm/lib/Support/Windows/Path.inc:404
 
-static std::error_code setDeleteDisposition(HANDLE Handle, bool Delete) {
+static std::error_code setDeleteDisposition(HANDLE Handle, bool &Delete) {
   // Clear the FILE_DISPOSITION_INFO flag first, before checking if it's a
----------------
Perhaps this variable should be renamed to `WillAutoDelete` or something like that? That might help express the meaning of changing its value. Although I'm not certain, since there's really two meanings - should delete and will auto delete.

(I don't particularly like the non-const input reference, but I don't see a better alternative at this point)


================
Comment at: llvm/lib/Support/Windows/Path.inc:1189
   if (Flags & OF_Delete) {
-    if ((EC = setDeleteDisposition(Result, true))) {
+    bool Delete = true;
+    if ((EC = setDeleteDisposition(Result, Delete))) {
----------------
Perhaps `ShouldDelete`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D111875/new/

https://reviews.llvm.org/D111875