[PATCH] D39449: Rewrite FileOutputBuffer as two separate classes.

[Zachary Turner via Phabricator via llvm-commits]

zturner added inline comments.


================
Comment at: llvm/lib/Support/FileOutputBuffer.cpp:84-85
+    std::error_code EC;
+    MemoryBlock MB = Memory::allocateMappedMemory(
+        Size, nullptr, sys::Memory::MF_READ | sys::Memory::MF_WRITE, EC);
+    if (EC)
----------------
Why can't we just use `new` here?


================
Comment at: llvm/lib/Support/FileOutputBuffer.cpp:145
+// Returns true if Path does not exist or is a regular file.
+static bool isFileRegular(StringRef Path, std::error_code &EC) {
+  fs::file_status Stat;
----------------
This is a bit confusing, in the sense that the function is called `isFileRegular` but then it can return true even if `is_regular_file` returns false.


================
Comment at: llvm/lib/Support/FileOutputBuffer.cpp:170
   std::error_code EC;
-  if (IsRegular) {
-    // Atomically replace the existing file with the new one.
-    EC = sys::fs::rename(Twine(TempPath), Twine(FinalPath));
-    sys::DontRemoveFileOnSignal(TempPath);
-  } else {
-    EC = sys::fs::copy_file(TempPath, FinalPath);
-    std::error_code RMEC = sys::fs::remove(TempPath);
-    sys::DontRemoveFileOnSignal(TempPath);
-    if (RMEC)
-      return RMEC;
-  }
+  bool IsRegular = isFileRegular(Path, EC);
+  if (EC)
----------------
It seems like we can just inline this.

```
fs::file_status Stat;
if (auto EC = fs::status(Path, Stat))
  return EC;
if (fs::is_directory(Stat))
  return errc::is_a_directory;

if (!fs::exists(Stat) || fs::is_regular_file(Stat))
  return OnDiskBuffer::create(Path, Size, Mode);
return InMemoryBuffer::create(Path, Size, Mode);
```

the extra function here just confuses things IMO.

That said, if it reaches the `InMemoryBuffer` code path, then what exactly *is* `Path`?  It definitely exists, but it's neither a file nor a directory.  So it's probably either a symlink, block file, character file, fifo, socket, or unknown file.  Why would  we be want to overwrite one of those with something?


https://reviews.llvm.org/D39449