[PATCH] D39449: Rewrite FileOutputBuffer as two separate classes.
Zachary Turner via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Oct 30 20:21:23 PDT 2017
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
More information about the llvm-commits
mailing list