TensorFlow: minor change functions in Env interface.

Switches to return a std::unique_ptr<> to convey transfer of ownership.
Keeps existing functions temporarily for backwards compatibility.

Change file_system.h interface to switch entirely to the new interface,
change all callers. If this breaks someone in the public, the interface
change should be straightforward.
Change: 125127028

7 files changed, 32 insertions, 37 deletions

diff --git a/tensorflow/core/util/events_writer.cc b/tensorflow/core/util/events_writer.cc
index c1f359c491..2a810f641e 100644
--- a/tensorflow/core/util/events_writer.cc
+++ b/tensorflow/core/util/events_writer.cc
@@ -57,13 +57,11 @@ bool EventsWriter::Init() {
       static_cast<long long>(time_in_seconds), port::Hostname().c_str());
   port::AdjustFilenameForLogging(&filename_);
 
-  WritableFile* file;
-  Status s = env_->NewWritableFile(filename_, &file);
+  Status s = env_->NewWritableFile(filename_, &recordio_file_);
   if (!s.ok()) {
     LOG(ERROR) << "Could not open events file: " << filename_ << ": " << s;
     return false;
   }
-  recordio_file_.reset(file);
   recordio_writer_.reset(new io::RecordWriter(recordio_file_.get()));
   if (recordio_writer_.get() == NULL) {
     LOG(ERROR) << "Could not create record writer";
diff --git a/tensorflow/core/util/memmapped_file_system.cc b/tensorflow/core/util/memmapped_file_system.cc
index a276421226..6d02595b8a 100644
--- a/tensorflow/core/util/memmapped_file_system.cc
+++ b/tensorflow/core/util/memmapped_file_system.cc
@@ -87,8 +87,8 @@ bool MemmappedFileSystem::FileExists(const string& fname) {
   return dir_element != directory_.end();
 }
 
-Status MemmappedFileSystem::NewRandomAccessFile(const string& filename,
-                                                RandomAccessFile** result) {
+Status MemmappedFileSystem::NewRandomAccessFile(
+    const string& filename, std::unique_ptr<RandomAccessFile>* result) {
   if (!mapped_memory_) {
     return errors::FailedPrecondition("MemmappedEnv is not initialized");
   }
@@ -96,14 +96,14 @@ Status MemmappedFileSystem::NewRandomAccessFile(const string& filename,
   if (dir_element == directory_.end()) {
     return errors::NotFound("Region ", filename, " is not found");
   }
-  *result = new RandomAccessFileFromMemmapped(
+  result->reset(new RandomAccessFileFromMemmapped(
       GetMemoryWithOffset(dir_element->second.offset),
-      dir_element->second.length);
+      dir_element->second.length));
   return Status::OK();
 }
 
 Status MemmappedFileSystem::NewReadOnlyMemoryRegionFromFile(
-    const string& filename, ReadOnlyMemoryRegion** result) {
+    const string& filename, std::unique_ptr<ReadOnlyMemoryRegion>* result) {
   if (!mapped_memory_) {
     return errors::FailedPrecondition("MemmappedEnv is not initialized");
   }
@@ -111,9 +111,9 @@ Status MemmappedFileSystem::NewReadOnlyMemoryRegionFromFile(
   if (dir_element == directory_.end()) {
     return errors::NotFound("Region ", filename, " is not found");
   }
-  *result = new ReadOnlyMemoryRegionFromMemmapped(
+  result->reset(new ReadOnlyMemoryRegionFromMemmapped(
       GetMemoryWithOffset(dir_element->second.offset),
-      dir_element->second.length);
+      dir_element->second.length));
   return Status::OK();
 }
 
@@ -130,12 +130,12 @@ Status MemmappedFileSystem::GetFileSize(const string& filename, uint64* size) {
 }
 
 Status MemmappedFileSystem::NewWritableFile(const string& filename,
-                                            WritableFile** wf) {
+                                            std::unique_ptr<WritableFile>* wf) {
   return errors::Unimplemented("memmapped format doesn't support writing");
 }
 
-Status MemmappedFileSystem::NewAppendableFile(const string& filename,
-                                              WritableFile** result) {
+Status MemmappedFileSystem::NewAppendableFile(
+    const string& filename, std::unique_ptr<WritableFile>* result) {
   return errors::Unimplemented("memmapped format doesn't support writing");
 }
 
@@ -170,9 +170,8 @@ constexpr char MemmappedFileSystem::kMemmappedPackageDefaultGraphDef[];
 
 Status MemmappedFileSystem::InitializeFromFile(Env* env,
                                                const string& filename) {
-  ReadOnlyMemoryRegion* region;
-  TF_RETURN_IF_ERROR(env->NewReadOnlyMemoryRegionFromFile(filename, &region));
-  mapped_memory_.reset(region);
+  TF_RETURN_IF_ERROR(
+      env->NewReadOnlyMemoryRegionFromFile(filename, &mapped_memory_));
   directory_.clear();
   if (mapped_memory_->length() <= sizeof(uint64)) {
     return errors::DataLoss("Corrupted memmapped model file: ", filename,
diff --git a/tensorflow/core/util/memmapped_file_system.h b/tensorflow/core/util/memmapped_file_system.h
index aaf18488a2..3c29579a3c 100644
--- a/tensorflow/core/util/memmapped_file_system.h
+++ b/tensorflow/core/util/memmapped_file_system.h
@@ -61,15 +61,19 @@ class MemmappedFileSystem : public FileSystem {
   MemmappedFileSystem();
   ~MemmappedFileSystem() override = default;
   bool FileExists(const string& fname) override;
-  Status NewRandomAccessFile(const string& filename,
-                             RandomAccessFile** result) override;
+  Status NewRandomAccessFile(
+      const string& filename,
+      std::unique_ptr<RandomAccessFile>* result) override;
   Status NewReadOnlyMemoryRegionFromFile(
-      const string& filename, ReadOnlyMemoryRegion** result) override;
+      const string& filename,
+      std::unique_ptr<ReadOnlyMemoryRegion>* result) override;
 
   // All these functions return Unimplemented error, the memmapped storage is
   // read only.
-  Status NewWritableFile(const string& fname, WritableFile** result) override;
-  Status NewAppendableFile(const string& fname, WritableFile** result) override;
+  Status NewWritableFile(const string& fname,
+                         std::unique_ptr<WritableFile>* result) override;
+  Status NewAppendableFile(const string& fname,
+                           std::unique_ptr<WritableFile>* result) override;
   Status GetChildren(const string& dir, std::vector<string>* r) override;
   Status DeleteFile(const string& f) override;
   Status CreateDir(const string& d) override;
diff --git a/tensorflow/core/util/memmapped_file_system_test.cc b/tensorflow/core/util/memmapped_file_system_test.cc
index 37408642a3..26e94f99f7 100644
--- a/tensorflow/core/util/memmapped_file_system_test.cc
+++ b/tensorflow/core/util/memmapped_file_system_test.cc
@@ -76,10 +76,10 @@ TEST(MemmappedFileSystemTest, SimpleTest) {
       ReadBinaryProto(&memmapped_env, kProtoFileName, &test_graph_def));
   EXPECT_EQ(kTestGraphDefVersion, test_graph_def.version());
   // Check that we can correctly get a tensor memory.
-  ReadOnlyMemoryRegion* memory_region;
+  std::unique_ptr<ReadOnlyMemoryRegion> memory_region;
   TF_ASSERT_OK(memmapped_env.NewReadOnlyMemoryRegionFromFile(kTensor2FileName,
                                                              &memory_region));
-  std::unique_ptr<ReadOnlyMemoryRegion> mem_region_ptr(memory_region);
+
   // The memory region can be bigger but not less than Tensor size.
   ASSERT_GE(memory_region->length(), test_tensor.TotalBytes());
   EXPECT_EQ(test_tensor.tensor_data(),
@@ -103,7 +103,7 @@ TEST(MemmappedFileSystemTest, SimpleTest) {
 
 TEST(MemmappedFileSystemTest, NotInitalized) {
   MemmappedEnv memmapped_env(Env::Default());
-  ReadOnlyMemoryRegion* memory_region;
+  std::unique_ptr<ReadOnlyMemoryRegion> memory_region;
   EXPECT_EQ(
       error::FAILED_PRECONDITION,
       memmapped_env
@@ -131,19 +131,17 @@ TEST(MemmappedFileSystemTest, ProxyToDefault) {
   const string dir = testing::TmpDir();
   const string filename = io::JoinPath(dir, "test_file");
   // Check that we can create write and read ordinary file.
-  WritableFile* writable_file;
+  std::unique_ptr<WritableFile> writable_file;
   TF_ASSERT_OK(memmapped_env.NewAppendableFile(filename, &writable_file));
-  std::unique_ptr<WritableFile> writable_file_ptr(writable_file);
   const string test_string = "bla-bla-bla";
   TF_ASSERT_OK(writable_file->Append(test_string));
   TF_ASSERT_OK(writable_file->Close());
   uint64 file_length = 0;
   TF_EXPECT_OK(memmapped_env.GetFileSize(filename, &file_length));
   EXPECT_EQ(test_string.length(), file_length);
-  RandomAccessFile* random_access_file;
+  std::unique_ptr<RandomAccessFile> random_access_file;
   TF_ASSERT_OK(
       memmapped_env.NewRandomAccessFile(filename, &random_access_file));
-  delete random_access_file;
 }
 
 }  // namespace
diff --git a/tensorflow/core/util/memmapped_file_system_writer.cc b/tensorflow/core/util/memmapped_file_system_writer.cc
index c4d6fa0440..7e87f4539c 100644
--- a/tensorflow/core/util/memmapped_file_system_writer.cc
+++ b/tensorflow/core/util/memmapped_file_system_writer.cc
@@ -20,10 +20,8 @@ namespace tensorflow {
 
 Status MemmappedFileSystemWriter::InitializeToFile(Env* env,
                                                    const string& filename) {
-  WritableFile* writable_file;
-  auto status = env->NewWritableFile(filename, &writable_file);
+  auto status = env->NewWritableFile(filename, &output_file_);
   if (status.ok()) {
-    output_file_.reset(writable_file);
     output_file_offset_ = 0;
   }
   return status;
diff --git a/tensorflow/core/util/reporter.cc b/tensorflow/core/util/reporter.cc
index 0cffcc2c93..a7e4b48175 100644
--- a/tensorflow/core/util/reporter.cc
+++ b/tensorflow/core/util/reporter.cc
@@ -58,10 +58,8 @@ Status TestReporter::Initialize() {
     return errors::InvalidArgument("Cannot create TestReporter, file exists: ",
                                    mangled_fname);
   }
-  WritableFile* log_file;
-  TF_RETURN_IF_ERROR(env->NewWritableFile(mangled_fname, &log_file));
-  log_file_.reset(log_file);
-  TF_RETURN_IF_ERROR(log_file->Flush());
+  TF_RETURN_IF_ERROR(env->NewWritableFile(mangled_fname, &log_file_));
+  TF_RETURN_IF_ERROR(log_file_->Flush());
 
   benchmark_entry_.set_name(test_name_);
   closed_ = false;
diff --git a/tensorflow/core/util/tensor_slice_writer.cc b/tensorflow/core/util/tensor_slice_writer.cc
index 74fcbbe649..bef421e8cf 100644
--- a/tensorflow/core/util/tensor_slice_writer.cc
+++ b/tensorflow/core/util/tensor_slice_writer.cc
@@ -68,10 +68,10 @@ class TableBuilder : public TensorSliceWriter::Builder {
 Status CreateTableTensorSliceBuilder(const string& name,
                                      TensorSliceWriter::Builder** builder) {
   *builder = nullptr;
-  WritableFile* f;
+  std::unique_ptr<WritableFile> f;
   Status s = Env::Default()->NewWritableFile(name, &f);
   if (s.ok()) {
-    *builder = new TableBuilder(name, f);
+    *builder = new TableBuilder(name, f.release());
     return Status::OK();
   } else {
     return s;