[llvm][Support] Deprecate llvm::writeFileAtomically API

[hokein]

Summary

The new llvm::writeToOutput API does the same thing, and we're in favor
of it.

This patch migrates 4 exiting usages and removes the llvm::writeFileAtomically API.

[hokein]

Let me know what you think about it -- I considered keeping the llvm::writeFileAtomically and migrating its underlying implementation to llvm::writeToOutput, but it doesn't seem to worth, there are only 4 in-tree usages of this API, I think it is probably better just remove it.

[avl]

added @jkorous who originally added llvm::writeFileAtomically.

Let me know what you think about it -- I considered keeping the llvm::writeFileAtomically and migrating its underlying implementation to llvm::writeToOutput, but it doesn't seem to worth, there are only 4 in-tree usages of this API, I think it is probably better just remove it.

I think it is OK to leave only one API (f.e. llvm::writeToOutput) and remove another. It would probably be better to split this patch to lldb, thinlto, clang and removal parts.

lldb/tools/lldb-server/lldb-platform.cpp

107 ↗

(On Diff #534444)

the call to llvm::writeToOutput is probably missed here.

[hokein]

In D153740#4448408, @avl wrote:

added @jkorous who originally added llvm::writeFileAtomically.

Let me know what you think about it -- I considered keeping the llvm::writeFileAtomically and migrating its underlying implementation to llvm::writeToOutput, but it doesn't seem to worth, there are only 4 in-tree usages of this API, I think it is probably better just remove it.

I think it is OK to leave only one API (f.e. llvm::writeToOutput) and remove another. It would probably be better to split this patch to lldb, thinlto, clang and removal parts.

Sounds good. I will split it once https://reviews.llvm.org/D153652 is landed.

[hokein]

Restrict the patch to only remove the writeFileAtomically API

[hokein]

Now this patch only contains the removal part of the API (I have cleaned all usages of writeFileAtomically API, except a remaining one in lldb https://reviews.llvm.org/D154329).

lldb/tools/lldb-server/lldb-platform.cpp

107 ↗

(On Diff #534444)

oops, good catch, the handleError should be writeToOutput.

[avl]

this LGTM, assuming D154329 is landed. Thank you!

llvm/include/llvm/Support/FileUtilities.h

void setFile(const Twine& filename, bool deleteIt = true) {
       DeleteIt = deleteIt;
     }
 
     /// releaseFile - Take ownership of the file away from the FileRemover so it
     /// will not be removed when the object is destroyed.
     void releaseFile() { DeleteIt = false; }
   };
 
-  enum class atomic_write_error {
-    failed_to_create_uniq_file = 0,
-    output_stream_error,
-    failed_to_rename_temp_file
-  };
-
-  class AtomicFileWriteError : public llvm::ErrorInfo<AtomicFileWriteError> {
-  public:
-    AtomicFileWriteError(atomic_write_error Error) : Error(Error) {}
-
-    void log(raw_ostream &OS) const override;
-
-    const atomic_write_error Error;
-    static char ID;
-
-  private:
-    // Users are not expected to use error_code.
-    std::error_code convertToErrorCode() const override {
-      return llvm::inconvertibleErrorCode();
-    }
-  };
-
-  // atomic_write_error + whatever the Writer can return
-
-  /// Creates a unique file with name according to the given \p TempPathModel,
-  /// writes content of \p Buffer to the file and renames it to \p FinalPath.
-  ///
-  /// \returns \c AtomicFileWriteError in case of error.
-  llvm::Error writeFileAtomically(StringRef TempPathModel, StringRef FinalPath,
-                                  StringRef Buffer);
-
-  llvm::Error
-  writeFileAtomically(StringRef TempPathModel, StringRef FinalPath,
-                      std::function<llvm::Error(llvm::raw_ostream &)> Writer);
-
   /// FilePermssionsApplier helps to copy permissions from an input file to
   /// an output one. It memorizes the status of the input file and can apply
   /// permissions and dates to the output file.
   class FilePermissionsApplier {
   public:
     static Expected<FilePermissionsApplier> create(StringRef InputFilename);
 
     /// Apply stored permissions to the \p OutputFilename.

llvm/lib/Support/FileUtilities.cpp

if (!CompareFailed && (!F1AtEnd || !F2AtEnd)) {
     // If we found the end, we succeeded.
     if (F1P < File1End || F2P < File2End)
       CompareFailed = true;
   }
 
   return CompareFailed;
 }
 
-void llvm::AtomicFileWriteError::log(raw_ostream &OS) const {
-  OS << "atomic_write_error: ";
-  switch (Error) {
-  case atomic_write_error::failed_to_create_uniq_file:
-    OS << "failed_to_create_uniq_file";
-    return;
-  case atomic_write_error::output_stream_error:
-    OS << "output_stream_error";
-    return;
-  case atomic_write_error::failed_to_rename_temp_file:
-    OS << "failed_to_rename_temp_file";
-    return;
-  }
-  llvm_unreachable("unknown atomic_write_error value in "
-                   "failed_to_rename_temp_file::log()");
-}
-
-llvm::Error llvm::writeFileAtomically(StringRef TempPathModel,
-                                      StringRef FinalPath, StringRef Buffer) {
-  return writeFileAtomically(TempPathModel, FinalPath,
-                             [&Buffer](llvm::raw_ostream &OS) {
-                               OS.write(Buffer.data(), Buffer.size());
-                               return llvm::Error::success();
-                             });
-}
-
-llvm::Error llvm::writeFileAtomically(
-    StringRef TempPathModel, StringRef FinalPath,
-    std::function<llvm::Error(llvm::raw_ostream &)> Writer) {
-  SmallString<128> GeneratedUniqPath;
-  int TempFD;
-  if (sys::fs::createUniqueFile(TempPathModel, TempFD, GeneratedUniqPath)) {
-    return llvm::make_error<AtomicFileWriteError>(
-        atomic_write_error::failed_to_create_uniq_file);
-  }
-  llvm::FileRemover RemoveTmpFileOnFail(GeneratedUniqPath);
-
-  raw_fd_ostream OS(TempFD, /*shouldClose=*/true);
-  if (llvm::Error Err = Writer(OS)) {
-    return Err;
-  }
-
-  OS.close();
-  if (OS.has_error()) {
-    OS.clear_error();
-    return llvm::make_error<AtomicFileWriteError>(
-        atomic_write_error::output_stream_error);
-  }
-
-  if (sys::fs::rename(/*from=*/GeneratedUniqPath, /*to=*/FinalPath)) {
-    return llvm::make_error<AtomicFileWriteError>(
-        atomic_write_error::failed_to_rename_temp_file);
-  }
-
-  RemoveTmpFileOnFail.releaseFile();
-  return Error::success();
-}
-
 Expected<FilePermissionsApplier>
 FilePermissionsApplier::create(StringRef InputFilename) {
   sys::fs::file_status Status;
 
   if (InputFilename != "-") {
     if (auto EC = sys::fs::status(InputFilename, Status))
       return createFileError(InputFilename, EC);
   } else {
#endif
       return createFileError(OutputFilename, EC);
   }
 
   if (std::error_code EC = sys::Process::SafelyCloseFileDescriptor(FD))
     return createFileError(OutputFilename, EC);
 
   return Error::success();
 }
-
-char llvm::AtomicFileWriteError::ID;

llvm/unittests/Support/CMakeLists.txt

add_llvm_unittest(SupportTests
   EndianTest.cpp
   ELFAttributeParserTest.cpp
   ErrnoTest.cpp
   ErrorOrTest.cpp
   ErrorTest.cpp
   ExtensibleRTTITest.cpp
   FileCollectorTest.cpp
   FileOutputBufferTest.cpp
-  FileUtilitiesTest.cpp
   FormatVariadicTest.cpp
   FSUniqueIDTest.cpp
   GlobPatternTest.cpp
   HashBuilderTest.cpp
   IndexedAccessorTest.cpp
   InstructionCostTest.cpp
   JSONTest.cpp
   KnownBitsTest.cpp

llvm/unittests/Support/FileUtilitiesTest.cpp

-//===- llvm/unittest/Support/FileUtilitiesTest.cpp - unit tests -----------===//
-//
-// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
-// See https://llvm.org/LICENSE.txt for license information.
-// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
-//
-//===----------------------------------------------------------------------===//
-
-#include "llvm/Support/FileUtilities.h"
-#include "llvm/Support/Errc.h"
-#include "llvm/Support/ErrorHandling.h"
-#include "llvm/Support/FileSystem.h"
-#include "llvm/Support/MemoryBuffer.h"
-#include "llvm/Support/Path.h"
-#include "llvm/Testing/Support/SupportHelpers.h"
-#include "gtest/gtest.h"
-#include <fstream>
-
-using namespace llvm;
-using namespace llvm::sys;
-
-using llvm::unittest::TempDir;
-
-#define ASSERT_NO_ERROR(x)                                                     \
-  if (std::error_code ASSERT_NO_ERROR_ec = x) {                                \
-    SmallString<128> MessageStorage;                                           \
-    raw_svector_ostream Message(MessageStorage);                               \
-    Message << #x ": did not return errc::success.\n"                          \
-            << "error number: " << ASSERT_NO_ERROR_ec.value() << "\n"          \
-            << "error message: " << ASSERT_NO_ERROR_ec.message() << "\n";      \
-    GTEST_FATAL_FAILURE_(MessageStorage.c_str());                              \
-  } else {                                                                     \
-  }
-
-namespace {
-TEST(writeFileAtomicallyTest, Test) {
-  // Create unique temporary directory for these tests
-  TempDir RootTestDirectory("writeFileAtomicallyTest", /*Unique*/ true);
-
-  SmallString<128> FinalTestfilePath(RootTestDirectory.path());
-  sys::path::append(FinalTestfilePath, "foo.txt");
-  const std::string TempUniqTestFileModel =
-      std::string(FinalTestfilePath) + "-%%%%%%%%";
-  const std::string TestfileContent = "fooFOOfoo";
-
-  llvm::Error Err = llvm::writeFileAtomically(TempUniqTestFileModel, FinalTestfilePath, TestfileContent);
-  ASSERT_FALSE(static_cast<bool>(Err));
-
-  std::ifstream FinalFileStream(std::string(FinalTestfilePath.str()));
-  std::string FinalFileContent;
-  FinalFileStream >> FinalFileContent;
-  ASSERT_EQ(FinalFileContent, TestfileContent);
-}
-} // anonymous namespace