From 0be06cf3609cdf6bdc8f769348f6ffcaba560eec Mon Sep 17 00:00:00 2001 From: Rafael Espindola Date: Thu, 11 Dec 2014 20:12:55 +0000 Subject: [PATCH] Remove a convoluted way of calling close by moving the call to the only caller. As a bonus we can actually check the return value. git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@224046 91177308-0d34-0410-b5e6-96231b3b80d8 --- include/llvm/Support/FileSystem.h | 12 ++----- lib/Support/FileOutputBuffer.cpp | 11 ++++++- lib/Support/MemoryBuffer.cpp | 2 +- lib/Support/Unix/Path.inc | 52 ++++--------------------------- lib/Support/Windows/Path.inc | 46 +++++---------------------- unittests/Support/Path.cpp | 15 ++------- 6 files changed, 30 insertions(+), 108 deletions(-) diff --git a/include/llvm/Support/FileSystem.h b/include/llvm/Support/FileSystem.h index 6c58e7b7db6..09561fcb0f2 100644 --- a/include/llvm/Support/FileSystem.h +++ b/include/llvm/Support/FileSystem.h @@ -637,7 +637,6 @@ public: private: /// Platform-specific mapping state. - mapmode Mode; uint64_t Size; void *Mapping; #ifdef LLVM_ON_WIN32 @@ -646,19 +645,14 @@ private: void *FileMappingHandle; #endif - std::error_code init(int FD, bool CloseFD, uint64_t Offset); + std::error_code init(int FD, uint64_t Offset, mapmode Mode); public: - typedef char char_type; - - mapped_file_region(mapped_file_region&&); - mapped_file_region &operator =(mapped_file_region&&); - /// \param fd An open file descriptor to map. mapped_file_region takes /// ownership if closefd is true. It must have been opended in the correct /// mode. - mapped_file_region(int fd, bool closefd, mapmode mode, uint64_t length, - uint64_t offset, std::error_code &ec); + mapped_file_region(int fd, mapmode mode, uint64_t length, uint64_t offset, + std::error_code &ec); ~mapped_file_region(); diff --git a/lib/Support/FileOutputBuffer.cpp b/lib/Support/FileOutputBuffer.cpp index c62655d58d5..57a0f60d277 100644 --- a/lib/Support/FileOutputBuffer.cpp +++ b/lib/Support/FileOutputBuffer.cpp @@ -18,6 +18,12 @@ #include "llvm/Support/raw_ostream.h" #include +#if !defined(_MSC_VER) && !defined(__MINGW32__) +#include +#else +#include +#endif + using llvm::sys::fs::mapped_file_region; namespace llvm { @@ -72,9 +78,12 @@ FileOutputBuffer::create(StringRef FilePath, size_t Size, return EC; auto MappedFile = llvm::make_unique( - FD, true, mapped_file_region::readwrite, Size, 0, EC); + FD, mapped_file_region::readwrite, Size, 0, EC); + int Ret = close(FD); if (EC) return EC; + if (Ret) + return std::error_code(errno, std::generic_category()); Result.reset( new FileOutputBuffer(std::move(MappedFile), FilePath, TempFilePath)); diff --git a/lib/Support/MemoryBuffer.cpp b/lib/Support/MemoryBuffer.cpp index 539b6c3e754..385ed6a5842 100644 --- a/lib/Support/MemoryBuffer.cpp +++ b/lib/Support/MemoryBuffer.cpp @@ -204,7 +204,7 @@ class MemoryBufferMMapFile : public MemoryBuffer { public: MemoryBufferMMapFile(bool RequiresNullTerminator, int FD, uint64_t Len, uint64_t Offset, std::error_code EC) - : MFR(FD, false, sys::fs::mapped_file_region::readonly, + : MFR(FD, sys::fs::mapped_file_region::readonly, getLegalMapSize(Len, Offset), getLegalMapOffset(Offset), EC) { if (!EC) { const char *Start = getStart(Len, Offset); diff --git a/lib/Support/Unix/Path.inc b/lib/Support/Unix/Path.inc index 72ef83cbcac..1e1b911b78b 100644 --- a/lib/Support/Unix/Path.inc +++ b/lib/Support/Unix/Path.inc @@ -62,31 +62,6 @@ using namespace llvm; -namespace { - /// This class automatically closes the given file descriptor when it goes out - /// of scope. You can take back explicit ownership of the file descriptor by - /// calling take(). The destructor does not verify that close was successful. - /// Therefore, never allow this class to call close on a file descriptor that - /// has been read from or written to. - struct AutoFD { - int FileDescriptor; - - AutoFD(int fd) : FileDescriptor(fd) {} - ~AutoFD() { - if (FileDescriptor >= 0) - ::close(FileDescriptor); - } - - int take() { - int ret = FileDescriptor; - FileDescriptor = -1; - return ret; - } - - operator int() const {return FileDescriptor;} - }; -} - namespace llvm { namespace sys { namespace fs { @@ -440,11 +415,8 @@ std::error_code setLastModificationAndAccessTime(int FD, TimeValue Time) { #endif } -std::error_code mapped_file_region::init(int FD, bool CloseFD, uint64_t Offset) { - AutoFD ScopedFD(FD); - if (!CloseFD) - ScopedFD.take(); - +std::error_code mapped_file_region::init(int FD, uint64_t Offset, + mapmode Mode) { // Figure out how large the file is. struct stat FileInfo; if (fstat(FD, &FileInfo) == -1) @@ -470,22 +442,16 @@ std::error_code mapped_file_region::init(int FD, bool CloseFD, uint64_t Offset) return std::error_code(); } -mapped_file_region::mapped_file_region(int fd, - bool closefd, - mapmode mode, - uint64_t length, - uint64_t offset, - std::error_code &ec) - : Mode(mode) - , Size(length) - , Mapping() { +mapped_file_region::mapped_file_region(int fd, mapmode mode, uint64_t length, + uint64_t offset, std::error_code &ec) + : Size(length), Mapping() { // Make sure that the requested size fits within SIZE_T. if (length > std::numeric_limits::max()) { ec = make_error_code(errc::invalid_argument); return; } - ec = init(fd, closefd, offset); + ec = init(fd, offset, mode); if (ec) Mapping = nullptr; } @@ -495,11 +461,6 @@ mapped_file_region::~mapped_file_region() { ::munmap(Mapping, Size); } -mapped_file_region::mapped_file_region(mapped_file_region &&other) - : Mode(other.Mode), Size(other.Size), Mapping(other.Mapping) { - other.Mapping = nullptr; -} - uint64_t mapped_file_region::size() const { assert(Mapping && "Mapping failed but used anyway!"); return Size; @@ -507,7 +468,6 @@ uint64_t mapped_file_region::size() const { char *mapped_file_region::data() const { assert(Mapping && "Mapping failed but used anyway!"); - assert(Mode != readonly && "Cannot get non-const data for readonly mapping!"); return reinterpret_cast(Mapping); } diff --git a/lib/Support/Windows/Path.inc b/lib/Support/Windows/Path.inc index eebee204a1e..20eae54b181 100644 --- a/lib/Support/Windows/Path.inc +++ b/lib/Support/Windows/Path.inc @@ -467,13 +467,12 @@ std::error_code setLastModificationAndAccessTime(int FD, TimeValue Time) { return std::error_code(); } -std::error_code mapped_file_region::init(int FD, bool CloseFD, uint64_t Offset) { +std::error_code mapped_file_region::init(int FD, uint64_t Offset, + mapmode Mode) { FileDescriptor = FD; // Make sure that the requested size fits within SIZE_T. if (Size > std::numeric_limits::max()) { if (FileDescriptor) { - if (CloseFD) - _close(FileDescriptor); } else ::CloseHandle(FileHandle); return make_error_code(errc::invalid_argument); @@ -494,8 +493,6 @@ std::error_code mapped_file_region::init(int FD, bool CloseFD, uint64_t Offset) if (FileMappingHandle == NULL) { std::error_code ec = windows_error(GetLastError()); if (FileDescriptor) { - if (CloseFD) - _close(FileDescriptor); } else ::CloseHandle(FileHandle); return ec; @@ -516,8 +513,6 @@ std::error_code mapped_file_region::init(int FD, bool CloseFD, uint64_t Offset) std::error_code ec = windows_error(GetLastError()); ::CloseHandle(FileMappingHandle); if (FileDescriptor) { - if (CloseFD) - _close(FileDescriptor); } else ::CloseHandle(FileHandle); return ec; @@ -531,8 +526,6 @@ std::error_code mapped_file_region::init(int FD, bool CloseFD, uint64_t Offset) ::UnmapViewOfFile(Mapping); ::CloseHandle(FileMappingHandle); if (FileDescriptor) { - if (CloseFD) - _close(FileDescriptor); } else ::CloseHandle(FileHandle); return ec; @@ -544,35 +537,23 @@ std::error_code mapped_file_region::init(int FD, bool CloseFD, uint64_t Offset) // alive. ::CloseHandle(FileMappingHandle); if (FileDescriptor) { - if (CloseFD) - _close(FileDescriptor); // Also closes FileHandle. } else ::CloseHandle(FileHandle); return std::error_code(); } -mapped_file_region::mapped_file_region(int fd, - bool closefd, - mapmode mode, - uint64_t length, - uint64_t offset, - std::error_code &ec) - : Mode(mode) - , Size(length) - , Mapping() - , FileDescriptor(fd) - , FileHandle(INVALID_HANDLE_VALUE) - , FileMappingHandle() { +mapped_file_region::mapped_file_region(int fd, mapmode mode, uint64_t length, + uint64_t offset, std::error_code &ec) + : Size(length), Mapping(), FileDescriptor(fd), + FileHandle(INVALID_HANDLE_VALUE), FileMappingHandle() { FileHandle = reinterpret_cast(_get_osfhandle(fd)); if (FileHandle == INVALID_HANDLE_VALUE) { - if (closefd) - _close(FileDescriptor); FileDescriptor = 0; ec = make_error_code(errc::bad_file_descriptor); return; } - ec = init(FileDescriptor, closefd, offset); + ec = init(FileDescriptor, offset, mode); if (ec) { Mapping = FileMappingHandle = 0; FileHandle = INVALID_HANDLE_VALUE; @@ -585,25 +566,12 @@ mapped_file_region::~mapped_file_region() { ::UnmapViewOfFile(Mapping); } -mapped_file_region::mapped_file_region(mapped_file_region &&other) - : Mode(other.Mode) - , Size(other.Size) - , Mapping(other.Mapping) - , FileDescriptor(other.FileDescriptor) - , FileHandle(other.FileHandle) - , FileMappingHandle(other.FileMappingHandle) { - other.Mapping = other.FileMappingHandle = 0; - other.FileHandle = INVALID_HANDLE_VALUE; - other.FileDescriptor = 0; -} - uint64_t mapped_file_region::size() const { assert(Mapping && "Mapping failed but used anyway!"); return Size; } char *mapped_file_region::data() const { - assert(Mode != readonly && "Cannot get non-const data for readonly mapping!"); assert(Mapping && "Mapping failed but used anyway!"); return reinterpret_cast(Mapping); } diff --git a/unittests/Support/Path.cpp b/unittests/Support/Path.cpp index d1f4e24ccb7..81d92342764 100644 --- a/unittests/Support/Path.cpp +++ b/unittests/Support/Path.cpp @@ -649,11 +649,7 @@ TEST_F(FileSystemTest, FileMapping) { StringRef Val("hello there"); { fs::mapped_file_region mfr(FileDescriptor, - true, - fs::mapped_file_region::readwrite, - 4096, - 0, - EC); + fs::mapped_file_region::readwrite, 4096, 0, EC); ASSERT_NO_ERROR(EC); std::copy(Val.begin(), Val.end(), mfr.data()); // Explicitly add a 0. @@ -665,21 +661,16 @@ TEST_F(FileSystemTest, FileMapping) { int FD; EC = fs::openFileForRead(Twine(TempPath), FD); ASSERT_NO_ERROR(EC); - fs::mapped_file_region mfr(FD, false, fs::mapped_file_region::readonly, 0, 0, - EC); + fs::mapped_file_region mfr(FD, fs::mapped_file_region::readonly, 0, 0, EC); ASSERT_NO_ERROR(EC); // Verify content EXPECT_EQ(StringRef(mfr.const_data()), Val); // Unmap temp file - fs::mapped_file_region m(FD, false, fs::mapped_file_region::readonly, 0, 0, - EC); + fs::mapped_file_region m(FD, fs::mapped_file_region::readonly, 0, 0, EC); ASSERT_NO_ERROR(EC); ASSERT_EQ(close(FD), 0); - const char *Data = m.const_data(); - fs::mapped_file_region mfrrv(std::move(m)); - EXPECT_EQ(mfrrv.const_data(), Data); } TEST(Support, NormalizePath) {