[index-while-buildling] FSUtil

[jkorous]

Summary

Just a utility function used in i-w-b.

[gribozavr]

clang/lib/IndexSerialization/FSUtil.h

27 ↗

(On Diff #217562)

"writeFileAtomically"

28 ↗

(On Diff #217562)

Please use StringRef instead of SmallString.

29 ↗

(On Diff #217562)

Why not llvm::Error?

35 ↗

(On Diff #217562)

failed to create a temporary file with model '...'

Traditionally after a colon one would write the reason why the operation failed, like "not enough space on disk"

62 ↗

(On Diff #217562)

Why not put it into llvm/Support/FileUtilities.h?

[jkorous]

Good points as always. Thanks!

[jkorous]

Addressed comments.

[jkorous]

Hmm, actually if we replace Buffer in the interface with a llvm::function_ref<void(llvm::raw_ostream &)> callback it would cover couple other existing uses of createUniqueFile.

[gribozavr]

In D66859#1649868, @jkorous wrote:

Hmm, actually if we replace Buffer in the interface with a llvm::function_ref<void(llvm::raw_ostream &)> callback it would cover couple other existing uses of createUniqueFile.

That would make a lot of sense if you're going to do the cleanup and refactor those callers to use new API. Good idea!

However, I think just from the API simplicity perspective, it would make sense to have two overloaded APIs if calling this function with a precomputed buffer is common.

[gribozavr]

Sorry for the late addition to the review, but any chance this API could have a unit test? Of course we can't test the automicity, but basic end to end flow on the "happy path" would be nice to test.

[jkorous]

Add test.

[jkorous]

In D66859#1650481, @gribozavr wrote:

In D66859#1649868, @jkorous wrote:

Hmm, actually if we replace Buffer in the interface with a llvm::function_ref<void(llvm::raw_ostream &)> callback it would cover couple other existing uses of createUniqueFile.

That would make a lot of sense if you're going to do the cleanup and refactor those callers to use new API. Good idea!

However, I think just from the API simplicity perspective, it would make sense to have two overloaded APIs if calling this function with a precomputed buffer is common.

Ok. I'll land this simple version first and will take a look at the overload next week.

llvm/trunk/include/llvm/Support/FileUtilities.h

 //===- llvm/Support/FileUtilities.h - File System Utilities -----*- C++ -*-===//
 //
 // 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
 //
 //===----------------------------------------------------------------------===//
 //
 // This file defines a family of utility functions which are useful for doing
 // various things with files.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_SUPPORT_FILEUTILITIES_H
 #define LLVM_SUPPORT_FILEUTILITIES_H
 
+#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/Errc.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/Path.h"
 
 namespace llvm {
 
   /// DiffFilesWithTolerance - Compare the two files specified, returning 0 if
   /// the files match, 1 if they are different, and 2 if there is a file error.
   /// This function allows you to specify an absolute and relative FP error that
void setFile(const Twine& filename, bool deleteIt = true) {
       filename.toVector(Filename);
       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; }
   };
+
+  /// 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.
+  llvm::Error writeFileAtomically(StringRef TempPathModel, StringRef FinalPath,
+                                  StringRef Buffer);
 } // End llvm namespace
 
 #endif

llvm/trunk/lib/Support/FileUtilities.cpp

 // various things with files.
 //
 //===----------------------------------------------------------------------===//
 
 #include "llvm/Support/FileUtilities.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/Support/ErrorOr.h"
 #include "llvm/Support/MemoryBuffer.h"
+#include "llvm/Support/Path.h"
 #include "llvm/Support/raw_ostream.h"
 #include <cctype>
 #include <cmath>
 #include <cstdint>
 #include <cstdlib>
 #include <cstring>
 #include <memory>
 #include <system_error>
if (!CompareFailed && (!F1AtEnd || !F2AtEnd)) {
 
     // If we found the end, we succeeded.
     if (F1P < File1End || F2P < File2End)
       CompareFailed = true;
   }
 
   return CompareFailed;
 }
+
+Error llvm::writeFileAtomically(StringRef TempPathModel, StringRef FinalPath,
+                                StringRef Buffer) {
+  SmallString<128> GeneratedUniqPath;
+  int TempFD;
+  if (const std::error_code Error = sys::fs::createUniqueFile(
+          TempPathModel.str(), TempFD, GeneratedUniqPath)) {
+    return createStringError(
+        Error, "failed to create temporary file with model \"%s\"",
+        TempPathModel.str().c_str());
+  }
+
+  raw_fd_ostream OS(TempFD, /*shouldClose=*/true);
+  OS.write(Buffer.data(), Buffer.size());
+  OS.close();
+  TempFD = -1;
+
+  if (OS.has_error()) {
+    const std::error_code Error = OS.error();
+    OS.clear_error();
+    return createStringError(Error, "failed to write to \"%s\"",
+                             GeneratedUniqPath.c_str());
+  }
+
+  if (const std::error_code Error =
+          sys::fs::rename(/*from=*/GeneratedUniqPath.c_str(),
+                          /*to=*/FinalPath.str().c_str())) {
+    return createStringError(Error, "failed to rename file \"%s\" to \"%s\"",
+                             GeneratedUniqPath.c_str(),
+                             FinalPath.str().c_str());
+  }
+
+  return Error::success();
+}

llvm/trunk/unittests/Support/CMakeLists.txt

add_llvm_unittest(SupportTests
   EndianStreamTest.cpp
   EndianTest.cpp
   ErrnoTest.cpp
   ErrorOrTest.cpp
   ErrorTest.cpp
   FileCheckTest.cpp
   FileCollectorTest.cpp
   FileOutputBufferTest.cpp
+  FileUtilitiesTest.cpp
   FormatVariadicTest.cpp
   GlobPatternTest.cpp
   Host.cpp
   ItaniumManglingCanonicalizerTest.cpp
   JSONTest.cpp
   KnownBitsTest.cpp
   LEB128Test.cpp
   LineIteratorTest.cpp

llvm/trunk/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 "gtest/gtest.h"
+#include <fstream>
+
+using namespace llvm;
+using namespace llvm::sys;
+
+#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
+  SmallString<128> RootTestDirectory;
+  ASSERT_NO_ERROR(
+    fs::createUniqueDirectory("writeFileAtomicallyTest", RootTestDirectory));
+
+  SmallString<128> FinalTestfilePath(RootTestDirectory);
+  sys::path::append(FinalTestfilePath, "foo.txt");
+  const std::string TempUniqTestFileModel = FinalTestfilePath.str().str() + "-%%%%%%%%";
+  const std::string TestfileContent = "fooFOOfoo";
+
+  llvm::Error Err = llvm::writeFileAtomically(TempUniqTestFileModel, FinalTestfilePath, TestfileContent);
+  ASSERT_FALSE(static_cast<bool>(Err));
+
+  std::ifstream FinalFileStream(FinalTestfilePath.str());
+  std::string FinalFileContent;
+  FinalFileStream >> FinalFileContent;
+  ASSERT_EQ(FinalFileContent, TestfileContent);
+}
+} // anonymous namespace