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

[Haojian Wu via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
hokein marked an inline comment as done.
Closed by commit rG7aafea001282: [llvm][Support] Deprecate 
llvm::writeFileAtomically API (authored by hokein).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D153740/new/

https://reviews.llvm.org/D153740

Files:
  llvm/include/llvm/Support/FileUtilities.h
  llvm/lib/Support/FileUtilities.cpp
  llvm/unittests/Support/CMakeLists.txt
  llvm/unittests/Support/FileUtilitiesTest.cpp

Index: llvm/unittests/Support/FileUtilitiesTest.cpp
===
--- llvm/unittests/Support/FileUtilitiesTest.cpp
+++ /dev/null
@@ -1,54 +0,0 @@
-//===- 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 
-
-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(Err));
-
-  std::ifstream FinalFileStream(std::string(FinalTestfilePath.str()));
-  std::string FinalFileContent;
-  FinalFileStream >> FinalFileContent;
-  ASSERT_EQ(FinalFileContent, TestfileContent);
-}
-} // anonymous namespace
Index: llvm/unittests/Support/CMakeLists.txt
===
--- llvm/unittests/Support/CMakeLists.txt
+++ llvm/unittests/Support/CMakeLists.txt
@@ -41,7 +41,6 @@
   ExtensibleRTTITest.cpp
   FileCollectorTest.cpp
   FileOutputBufferTest.cpp
-  FileUtilitiesTest.cpp
   FormatVariadicTest.cpp
   FSUniqueIDTest.cpp
   GlobPatternTest.cpp
Index: llvm/lib/Support/FileUtilities.cpp
===
--- llvm/lib/Support/FileUtilities.cpp
+++ llvm/lib/Support/FileUtilities.cpp
@@ -267,64 +267,6 @@
   return CompareFailed;
 }
 
-void llvm::AtomicFileWriteError::log(raw_ostream ) 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,
- [](llvm::raw_ostream ) {
-   OS.write(Buffer.data(), Buffer.size());
-   return llvm::Error::success();
- });
-}
-
-llvm::Error llvm::writeFileAtomically(
-StringRef TempPathModel, StringRef FinalPath,
-std::function Writer) {
-  SmallString<128> GeneratedUniqPath;
-  int TempFD;
-  if (sys::fs::createUniqueFile(TempPathModel, TempFD, 

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

[Alexey Lapshin via Phabricator via cfe-commits]

avl accepted this revision.
avl added a comment.
This revision is now accepted and ready to land.

this LGTM, assuming D154329  is landed. Thank 
you!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D153740/new/

https://reviews.llvm.org/D153740

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Haojian Wu via Phabricator via cfe-commits]

hokein marked an inline comment as done.
hokein added a comment.

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).




Comment at: lldb/tools/lldb-server/lldb-platform.cpp:107
   Status status;
-  if (auto Err =
-  handleErrors(llvm::writeFileAtomically(
-   temp_file_path, file_spec.GetPath(), socket_id),
-   [, _spec](const AtomicFileWriteError ) {
- std::string ErrorMsgBuffer;
- llvm::raw_string_ostream S(ErrorMsgBuffer);
- E.log(S);
-
- switch (E.Error) {
- case atomic_write_error::failed_to_create_uniq_file:
-   status = Status("Failed to create temp file: %s",
-   ErrorMsgBuffer.c_str());
-   break;
- case atomic_write_error::output_stream_error:
-   status = Status("Failed to write to port file.");
-   break;
- case atomic_write_error::failed_to_rename_temp_file:
-   status = Status("Failed to rename file %s to %s: 
%s",
-   ErrorMsgBuffer.c_str(),
-   file_spec.GetPath().c_str(),
-   ErrorMsgBuffer.c_str());
-   break;
- }
-   })) {
+  if (auto Err = handleErrors(file_spec.GetPath(),
+  [_id](llvm::raw_ostream ) {

avl wrote:
> the call to llvm::writeToOutput is probably missed here.
oops, good catch, the `handleError` should be `writeToOutput`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D153740/new/

https://reviews.llvm.org/D153740

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Haojian Wu via Phabricator via cfe-commits]

hokein updated this revision to Diff 536698.
hokein added a comment.

Restrict the patch to only remove the writeFileAtomically API


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D153740/new/

https://reviews.llvm.org/D153740

Files:
  llvm/include/llvm/Support/FileUtilities.h
  llvm/lib/Support/FileUtilities.cpp
  llvm/unittests/Support/CMakeLists.txt
  llvm/unittests/Support/FileUtilitiesTest.cpp

Index: llvm/unittests/Support/FileUtilitiesTest.cpp
===
--- llvm/unittests/Support/FileUtilitiesTest.cpp
+++ /dev/null
@@ -1,54 +0,0 @@
-//===- 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 
-
-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(Err));
-
-  std::ifstream FinalFileStream(std::string(FinalTestfilePath.str()));
-  std::string FinalFileContent;
-  FinalFileStream >> FinalFileContent;
-  ASSERT_EQ(FinalFileContent, TestfileContent);
-}
-} // anonymous namespace
Index: llvm/unittests/Support/CMakeLists.txt
===
--- llvm/unittests/Support/CMakeLists.txt
+++ llvm/unittests/Support/CMakeLists.txt
@@ -41,7 +41,6 @@
   ExtensibleRTTITest.cpp
   FileCollectorTest.cpp
   FileOutputBufferTest.cpp
-  FileUtilitiesTest.cpp
   FormatVariadicTest.cpp
   FSUniqueIDTest.cpp
   GlobPatternTest.cpp
Index: llvm/lib/Support/FileUtilities.cpp
===
--- llvm/lib/Support/FileUtilities.cpp
+++ llvm/lib/Support/FileUtilities.cpp
@@ -267,64 +267,6 @@
   return CompareFailed;
 }
 
-void llvm::AtomicFileWriteError::log(raw_ostream ) 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,
- [](llvm::raw_ostream ) {
-   OS.write(Buffer.data(), Buffer.size());
-   return llvm::Error::success();
- });
-}
-
-llvm::Error llvm::writeFileAtomically(
-StringRef TempPathModel, StringRef FinalPath,
-std::function Writer) {
-  SmallString<128> GeneratedUniqPath;
-  int TempFD;
-  if (sys::fs::createUniqueFile(TempPathModel, TempFD, GeneratedUniqPath)) {
-return llvm::make_error(
-atomic_write_error::failed_to_create_uniq_file);
-  }
-  llvm::FileRemover 

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

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D153740/new/

https://reviews.llvm.org/D153740

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Alexey Lapshin via Phabricator via cfe-commits]

avl added a reviewer: jkorous.
avl added a subscriber: jkorous.
avl added a comment.

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.




Comment at: lldb/tools/lldb-server/lldb-platform.cpp:107
   Status status;
-  if (auto Err =
-  handleErrors(llvm::writeFileAtomically(
-   temp_file_path, file_spec.GetPath(), socket_id),
-   [, _spec](const AtomicFileWriteError ) {
- std::string ErrorMsgBuffer;
- llvm::raw_string_ostream S(ErrorMsgBuffer);
- E.log(S);
-
- switch (E.Error) {
- case atomic_write_error::failed_to_create_uniq_file:
-   status = Status("Failed to create temp file: %s",
-   ErrorMsgBuffer.c_str());
-   break;
- case atomic_write_error::output_stream_error:
-   status = Status("Failed to write to port file.");
-   break;
- case atomic_write_error::failed_to_rename_temp_file:
-   status = Status("Failed to rename file %s to %s: 
%s",
-   ErrorMsgBuffer.c_str(),
-   file_spec.GetPath().c_str(),
-   ErrorMsgBuffer.c_str());
-   break;
- }
-   })) {
+  if (auto Err = handleErrors(file_spec.GetPath(),
+  [_id](llvm::raw_ostream ) {

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D153740/new/

https://reviews.llvm.org/D153740

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.
Herald added a subscriber: JDevlieghere.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D153740/new/

https://reviews.llvm.org/D153740

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Haojian Wu via Phabricator via cfe-commits]

hokein created this revision.
hokein added a reviewer: avl.
Herald added subscribers: ormris, kadircet, arphaman, steven_wu, hiraditya.
Herald added a project: All.
hokein requested review of this revision.
Herald added projects: clang, LLDB, LLVM, clang-tools-extra.
Herald added subscribers: llvm-commits, lldb-commits.

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.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D153740

Files:
  clang-tools-extra/clangd/index/BackgroundIndexStorage.cpp
  clang/lib/Frontend/ASTUnit.cpp
  clang/lib/Serialization/GlobalModuleIndex.cpp
  lldb/tools/lldb-server/lldb-platform.cpp
  llvm/include/llvm/Support/FileUtilities.h
  llvm/lib/LTO/ThinLTOCodeGenerator.cpp
  llvm/lib/Support/FileUtilities.cpp
  llvm/unittests/Support/CMakeLists.txt
  llvm/unittests/Support/FileUtilitiesTest.cpp

Index: llvm/unittests/Support/FileUtilitiesTest.cpp
===
--- llvm/unittests/Support/FileUtilitiesTest.cpp
+++ /dev/null
@@ -1,54 +0,0 @@
-//===- 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 
-
-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(Err));
-
-  std::ifstream FinalFileStream(std::string(FinalTestfilePath.str()));
-  std::string FinalFileContent;
-  FinalFileStream >> FinalFileContent;
-  ASSERT_EQ(FinalFileContent, TestfileContent);
-}
-} // anonymous namespace
Index: llvm/unittests/Support/CMakeLists.txt
===
--- llvm/unittests/Support/CMakeLists.txt
+++ llvm/unittests/Support/CMakeLists.txt
@@ -41,7 +41,6 @@
   ExtensibleRTTITest.cpp
   FileCollectorTest.cpp
   FileOutputBufferTest.cpp
-  FileUtilitiesTest.cpp
   FormatVariadicTest.cpp
   FSUniqueIDTest.cpp
   GlobPatternTest.cpp
Index: llvm/lib/Support/FileUtilities.cpp
===
--- llvm/lib/Support/FileUtilities.cpp
+++ llvm/lib/Support/FileUtilities.cpp
@@ -267,64 +267,6 @@
   return CompareFailed;
 }
 
-void llvm::AtomicFileWriteError::log(raw_ostream ) 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,
- [](llvm::raw_ostream ) {
-