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.

llvm-svn: 224046

llvm/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,
+  mapped_file_region(int fd, mapmode mode, uint64_t length, uint64_t offset,
-                     uint64_t offset, std::error_code &ec);
+                     std::error_code &ec);
 
   ~mapped_file_region();
 

llvm/lib/Support/FileOutputBuffer.cpp

@@ -18,6 +18,12 @@
 #include "llvm/Support/raw_ostream.h"
 #include <system_error>
 
+#if !defined(_MSC_VER) && !defined(__MINGW32__)
+#include <unistd.h>
+#else
+#include <io.h>
+#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<mapped_file_region>(
-      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));

llvm/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);

llvm/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) {
+std::error_code mapped_file_region::init(int FD, uint64_t Offset,
-  AutoFD ScopedFD(FD);
+                                         mapmode Mode) {
-  if (!CloseFD)
-    ScopedFD.take();
-
   // 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,
+mapped_file_region::mapped_file_region(int fd, mapmode mode, uint64_t length,
-                                       bool closefd,
+                                       uint64_t offset, std::error_code &ec)
-                                       mapmode mode,
+    : Size(length), Mapping() {
-                                       uint64_t length,
-                                       uint64_t offset,
-                                       std::error_code &ec)
-  : Mode(mode)
-  , Size(length)
-  , Mapping() {
   // Make sure that the requested size fits within SIZE_T.
   if (length > std::numeric_limits<size_t>::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<char*>(Mapping);
 }
 

llvm/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<SIZE_T>::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,
+mapped_file_region::mapped_file_region(int fd, mapmode mode, uint64_t length,
-                                       bool closefd,
+                                       uint64_t offset, std::error_code &ec)
-                                       mapmode mode,
+    : Size(length), Mapping(), FileDescriptor(fd),
-                                       uint64_t length,
+      FileHandle(INVALID_HANDLE_VALUE), FileMappingHandle() {
-                                       uint64_t offset,
-                                       std::error_code &ec)
-  : Mode(mode)
-  , Size(length)
-  , Mapping()
-  , FileDescriptor(fd)
-  , FileHandle(INVALID_HANDLE_VALUE)
-  , FileMappingHandle() {
   FileHandle = reinterpret_cast<HANDLE>(_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<char*>(Mapping);
 }

llvm/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,
+  fs::mapped_file_region mfr(FD, fs::mapped_file_region::readonly, 0, 0, EC);
-                             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,
+  fs::mapped_file_region m(FD, fs::mapped_file_region::readonly, 0, 0, EC);
-                           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) {