Add Env::Remove{File,Dir} which obsolete Env::Delete{File,Dir}.

The "DeleteFile" method name causes pain for Windows developers, because
<windows.h> #defines a DeleteFile macro to DeleteFileW or DeleteFileA.
Current code uses workarounds, like #undefining DeleteFile everywhere an
Env is declared, implemented, or used.

This CL removes the need for workarounds by renaming Env::DeleteFile to
Env::RemoveFile. For consistency, Env::DeleteDir is also renamed to
Env::RemoveDir. A few internal methods are also renamed for consistency.
Software that supports Windows is expected to migrate any Env
implementations and usage to Remove{File,Dir}, and never use the name
Env::Delete{File,Dir} in its code.

The renaming is done in a backwards-compatible way, at the risk of
making it slightly more difficult to build a new correct Env
implementation. The backwards compatibility is achieved using the
following hacks:

1) Env::Remove{File,Dir} methods are added, with a default
    implementation that calls into Env::Delete{File,Dir}. This makes old
    Env implementations compatible with code that calls into the updated
    API.
2) The Env::Delete{File,Dir} methods are no longer pure virtuals.
    Instead, they gain a default implementation that calls into
    Env::Remove{File,Dir}. This makes updated Env implementations
    compatible with code that calls into the old API.

The cost of this approach is that it's possible to write an Env without
overriding either Rename{File,Dir} or Delete{File,Dir}, without getting
a compiler warning. However, attempting to run the test suite will
immediately fail with an infinite call stack ending in
{Remove,Delete}{File,Dir}, making developers aware of the problem.

PiperOrigin-RevId: 288710907

1 files changed, 52 insertions, 21 deletions

diff --git a/include/leveldb/env.h b/include/leveldb/env.h
index 112fe96..6501fa4 100644
--- a/include/leveldb/env.h
+++ b/include/leveldb/env.h
@@ -22,21 +22,18 @@
 #include "leveldb/export.h"
 #include "leveldb/status.h"
 
+// This workaround can be removed when leveldb::Env::DeleteFile is removed.
 #if defined(_WIN32)
-// The leveldb::Env class below contains a DeleteFile method.
-// At the same time, <windows.h>, a fairly popular header
-// file for Windows applications, defines a DeleteFile macro.
+// On Windows, the method name DeleteFile (below) introduces the risk of
+// triggering undefined behavior by exposing the compiler to different
+// declarations of the Env class in different translation units.
 //
-// Without any intervention on our part, the result of this
-// unfortunate coincidence is that the name of the
-// leveldb::Env::DeleteFile method seen by the compiler depends on
-// whether <windows.h> was included before or after the LevelDB
-// headers.
+// This is because <windows.h>, a fairly popular header file for Windows
+// applications, defines a DeleteFile macro. So, files that include the Windows
+// header before this header will contain an altered Env declaration.
 //
-// To avoid headaches, we undefined DeleteFile (if defined) and
-// redefine it at the bottom of this file. This way <windows.h>
-// can be included before this file (or not at all) and the
-// exported method will always be leveldb::Env::DeleteFile.
+// This workaround ensures that the compiler sees the same Env declaration,
+// independently of whether <windows.h> was included.
 #if defined(DeleteFile)
 #undef DeleteFile
 #define LEVELDB_DELETEFILE_UNDEFINED
@@ -54,7 +51,7 @@ class WritableFile;
 
 class LEVELDB_EXPORT Env {
  public:
-  Env() = default;
+  Env();
 
   Env(const Env&) = delete;
   Env& operator=(const Env&) = delete;
@@ -122,15 +119,48 @@ class LEVELDB_EXPORT Env {
   // Original contents of *results are dropped.
   virtual Status GetChildren(const std::string& dir,
                              std::vector<std::string>* result) = 0;
-
   // Delete the named file.
-  virtual Status DeleteFile(const std::string& fname) = 0;
+  //
+  // The default implementation calls DeleteFile, to support legacy Env
+  // implementations. Updated Env implementations must override RemoveFile and
+  // ignore the existence of DeleteFile. Updated code calling into the Env API
+  // must call RemoveFile instead of DeleteFile.
+  //
+  // A future release will remove DeleteDir and the default implementation of
+  // RemoveDir.
+  virtual Status RemoveFile(const std::string& fname);
+
+  // DEPRECATED: Modern Env implementations should override RemoveFile instead.
+  //
+  // The default implementation calls RemoveFile, to support legacy Env user
+  // code that calls this method on modern Env implementations. Modern Env user
+  // code should call RemoveFile.
+  //
+  // A future release will remove this method.
+  virtual Status DeleteFile(const std::string& fname);
 
   // Create the specified directory.
   virtual Status CreateDir(const std::string& dirname) = 0;
 
   // Delete the specified directory.
-  virtual Status DeleteDir(const std::string& dirname) = 0;
+  //
+  // The default implementation calls DeleteDir, to support legacy Env
+  // implementations. Updated Env implementations must override RemoveDir and
+  // ignore the existence of DeleteDir. Modern code calling into the Env API
+  // must call RemoveDir instead of DeleteDir.
+  //
+  // A future release will remove DeleteDir and the default implementation of
+  // RemoveDir.
+  virtual Status RemoveDir(const std::string& dirname);
+
+  // DEPRECATED: Modern Env implementations should override RemoveDir instead.
+  //
+  // The default implementation calls RemoveDir, to support legacy Env user
+  // code that calls this method on modern Env implementations. Modern Env user
+  // code should call RemoveDir.
+  //
+  // A future release will remove this method.
+  virtual Status DeleteDir(const std::string& dirname);
 
   // Store the size of fname in *file_size.
   virtual Status GetFileSize(const std::string& fname, uint64_t* file_size) = 0;
@@ -333,14 +363,14 @@ class LEVELDB_EXPORT EnvWrapper : public Env {
                      std::vector<std::string>* r) override {
     return target_->GetChildren(dir, r);
   }
-  Status DeleteFile(const std::string& f) override {
-    return target_->DeleteFile(f);
+  Status RemoveFile(const std::string& f) override {
+    return target_->RemoveFile(f);
   }
   Status CreateDir(const std::string& d) override {
     return target_->CreateDir(d);
   }
-  Status DeleteDir(const std::string& d) override {
-    return target_->DeleteDir(d);
+  Status RemoveDir(const std::string& d) override {
+    return target_->RemoveDir(d);
   }
   Status GetFileSize(const std::string& f, uint64_t* s) override {
     return target_->GetFileSize(f, s);
@@ -375,7 +405,8 @@ class LEVELDB_EXPORT EnvWrapper : public Env {
 
 }  // namespace leveldb
 
-// Redefine DeleteFile if necessary.
+// This workaround can be removed when leveldb::Env::DeleteFile is removed.
+// Redefine DeleteFile if it was undefined earlier.
 #if defined(_WIN32) && defined(LEVELDB_DELETEFILE_UNDEFINED)
 #if defined(UNICODE)
 #define DeleteFile DeleteFileW