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

[Rui Ueyama via Phabricator via llvm-commits]

ruiu 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)
----------------
zturner wrote:
> Why can't we just use `new` here?
I didn't design `Memory` class, but it looks like that class can be instantiated only via allocateMappedMemory.


================
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)
----------------
zturner wrote:
> 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?
Thank you for the suggestion. Your code looks indeed better. Updated the patch. Also added comment to answer your question.


https://reviews.llvm.org/D39449