[Mlir-commits] [mlir] 48cd9d9 - [Support] Use outs() in ToolOutputFile
Pavel Labath
llvmlistbot at llvm.org
Thu Jun 4 05:56:46 PDT 2020
Author: Pavel Labath
Date: 2020-06-04T14:56:35+02:00
New Revision: 48cd9d9dd86c2c37e6c47cbb23c06d36a1870b83
URL: https://github.com/llvm/llvm-project/commit/48cd9d9dd86c2c37e6c47cbb23c06d36a1870b83
DIFF: https://github.com/llvm/llvm-project/commit/48cd9d9dd86c2c37e6c47cbb23c06d36a1870b83.diff
LOG: [Support] Use outs() in ToolOutputFile
Summary:
If the output filename was specified as "-", the ToolOutputFile class
would create a brand new raw_ostream object referring to the stdout.
This patch changes it to reuse the llvm::outs() singleton.
At the moment, this change should be "NFC", but it does enable other
enhancements, like the automatic stdout/stderr synchronization as
discussed on D80803.
I've checked the history, and I did not find any indication that this
class *has* to use a brand new stream object instead of outs() --
indeed, it is special-casing "-" in a number of places already, so this
change fits the pattern pretty well. I suspect the main reason for the
current state of affairs is that the class was originally introduced
(r111595, in 2010) as a raw_fd_ostream subclass, which made any other
solution impossible.
Another potential benefit of this patch is that it makes it possible to
move the raw_ostream class out of the business of special-casing "-" for
stdout handling. That state of affairs does not seem appropriate because
"-" is a valid filename (albeit hard to access with a lot of command
line tools) on most systems. Handling "-" in ToolOutputFile seems more
appropriate.
To make this possible, this patch changes the return type of
llvm::outs() and errs() to raw_fd_ostream&. Previously the functions
were constructing objects of that type, but returning a generic
raw_ostream reference. This makes it possible for new ToolOutputFile and
other code to use raw_fd_ostream methods like error() on the outs()
object. This does not seem like a bad thing (since stdout is a file
descriptor which can be redirected to anywhere, it makes sense to ask it
whether the writing was successful or if it supports seeking), and
indeed a lot of code was already depending on this fact via the
ToolOutputFile "back door".
Reviewers: dblaikie, JDevlieghere, MaskRay, jhenderson
Subscribers: hiraditya, llvm-commits
Tags: #llvm
Differential Revision: https://reviews.llvm.org/D81078
Added:
llvm/unittests/Support/ToolOutputFileTest.cpp
Modified:
llvm/include/llvm/Support/ToolOutputFile.h
llvm/include/llvm/Support/raw_ostream.h
llvm/lib/Support/ToolOutputFile.cpp
llvm/lib/Support/raw_ostream.cpp
llvm/unittests/Support/CMakeLists.txt
mlir/include/mlir/Pass/PassManager.h
Removed:
################################################################################
diff --git a/llvm/include/llvm/Support/ToolOutputFile.h b/llvm/include/llvm/Support/ToolOutputFile.h
index a99e327f8db7..cf01b9ecefc5 100644
--- a/llvm/include/llvm/Support/ToolOutputFile.h
+++ b/llvm/include/llvm/Support/ToolOutputFile.h
@@ -13,6 +13,7 @@
#ifndef LLVM_SUPPORT_TOOLOUTPUTFILE_H
#define LLVM_SUPPORT_TOOLOUTPUTFILE_H
+#include "llvm/ADT/Optional.h"
#include "llvm/Support/raw_ostream.h"
namespace llvm {
@@ -38,8 +39,12 @@ class ToolOutputFile {
~CleanupInstaller();
} Installer;
- /// The contained stream. This is intentionally declared after Installer.
- raw_fd_ostream OS;
+ /// Storage for the stream, if we're owning our own stream. This is
+ /// intentionally declared after Installer.
+ Optional<raw_fd_ostream> OSHolder;
+
+ /// The actual stream to use.
+ raw_fd_ostream *OS;
public:
/// This constructor's arguments are passed to raw_fd_ostream's
@@ -50,7 +55,7 @@ class ToolOutputFile {
ToolOutputFile(StringRef Filename, int FD);
/// Return the contained raw_fd_ostream.
- raw_fd_ostream &os() { return OS; }
+ raw_fd_ostream &os() { return *OS; }
/// Indicate that the tool's job wrt this output file has been successful and
/// the file should not be deleted.
diff --git a/llvm/include/llvm/Support/raw_ostream.h b/llvm/include/llvm/Support/raw_ostream.h
index 72cedc2a6065..00d82dabfcdd 100644
--- a/llvm/include/llvm/Support/raw_ostream.h
+++ b/llvm/include/llvm/Support/raw_ostream.h
@@ -496,13 +496,13 @@ class raw_fd_ostream : public raw_pwrite_stream {
void clear_error() { EC = std::error_code(); }
};
-/// This returns a reference to a raw_ostream for standard output. Use it like:
-/// outs() << "foo" << "bar";
-raw_ostream &outs();
+/// This returns a reference to a raw_fd_ostream for standard output. Use it
+/// like: outs() << "foo" << "bar";
+raw_fd_ostream &outs();
-/// This returns a reference to a raw_ostream for standard error. Use it like:
-/// errs() << "foo" << "bar";
-raw_ostream &errs();
+/// This returns a reference to a raw_fd_ostream for standard error. Use it
+/// like: errs() << "foo" << "bar";
+raw_fd_ostream &errs();
/// This returns a reference to a raw_ostream which simply discards output.
raw_ostream &nulls();
diff --git a/llvm/lib/Support/ToolOutputFile.cpp b/llvm/lib/Support/ToolOutputFile.cpp
index 1051af3eb45b..c2ca97a59c62 100644
--- a/llvm/lib/Support/ToolOutputFile.cpp
+++ b/llvm/lib/Support/ToolOutputFile.cpp
@@ -15,31 +15,45 @@
#include "llvm/Support/Signals.h"
using namespace llvm;
+static bool isStdout(StringRef Filename) { return Filename == "-"; }
+
ToolOutputFile::CleanupInstaller::CleanupInstaller(StringRef Filename)
: Filename(std::string(Filename)), Keep(false) {
// Arrange for the file to be deleted if the process is killed.
- if (Filename != "-")
+ if (!isStdout(Filename))
sys::RemoveFileOnSignal(Filename);
}
ToolOutputFile::CleanupInstaller::~CleanupInstaller() {
+ if (isStdout(Filename))
+ return;
+
// Delete the file if the client hasn't told us not to.
- if (!Keep && Filename != "-")
+ if (!Keep)
sys::fs::remove(Filename);
// Ok, the file is successfully written and closed, or deleted. There's no
// further need to clean it up on signals.
- if (Filename != "-")
- sys::DontRemoveFileOnSignal(Filename);
+ sys::DontRemoveFileOnSignal(Filename);
}
ToolOutputFile::ToolOutputFile(StringRef Filename, std::error_code &EC,
sys::fs::OpenFlags Flags)
- : Installer(Filename), OS(Filename, EC, Flags) {
+ : Installer(Filename) {
+ if (isStdout(Filename)) {
+ OS = &outs();
+ EC = std::error_code();
+ return;
+ }
+ OSHolder.emplace(Filename, EC, Flags);
+ OS = OSHolder.getPointer();
// If open fails, no cleanup is needed.
if (EC)
Installer.Keep = true;
}
ToolOutputFile::ToolOutputFile(StringRef Filename, int FD)
- : Installer(Filename), OS(FD, true) {}
+ : Installer(Filename) {
+ OSHolder.emplace(FD, true);
+ OS = OSHolder.getPointer();
+}
diff --git a/llvm/lib/Support/raw_ostream.cpp b/llvm/lib/Support/raw_ostream.cpp
index 7cac834f1010..cb586a46cdee 100644
--- a/llvm/lib/Support/raw_ostream.cpp
+++ b/llvm/lib/Support/raw_ostream.cpp
@@ -864,9 +864,7 @@ void raw_fd_ostream::anchor() {}
// outs(), errs(), nulls()
//===----------------------------------------------------------------------===//
-/// outs() - This returns a reference to a raw_ostream for standard output.
-/// Use it like: outs() << "foo" << "bar";
-raw_ostream &llvm::outs() {
+raw_fd_ostream &llvm::outs() {
// Set buffer settings to model stdout behavior.
std::error_code EC;
static raw_fd_ostream S("-", EC, sys::fs::OF_None);
@@ -874,9 +872,7 @@ raw_ostream &llvm::outs() {
return S;
}
-/// errs() - This returns a reference to a raw_ostream for standard error.
-/// Use it like: errs() << "foo" << "bar";
-raw_ostream &llvm::errs() {
+raw_fd_ostream &llvm::errs() {
// Set standard error to be unbuffered by default.
static raw_fd_ostream S(STDERR_FILENO, false, true);
return S;
diff --git a/llvm/unittests/Support/CMakeLists.txt b/llvm/unittests/Support/CMakeLists.txt
index 75ca0e74f193..d343d6dd08bf 100644
--- a/llvm/unittests/Support/CMakeLists.txt
+++ b/llvm/unittests/Support/CMakeLists.txt
@@ -75,6 +75,7 @@ add_llvm_unittest(SupportTests
ThreadPool.cpp
Threading.cpp
TimerTest.cpp
+ ToolOutputFileTest.cpp
TypeNameTest.cpp
TypeTraitsTest.cpp
TrailingObjectsTest.cpp
diff --git a/llvm/unittests/Support/ToolOutputFileTest.cpp b/llvm/unittests/Support/ToolOutputFileTest.cpp
new file mode 100644
index 000000000000..853136b06ee4
--- /dev/null
+++ b/llvm/unittests/Support/ToolOutputFileTest.cpp
@@ -0,0 +1,22 @@
+//===- ToolOutputFileTest.cpp - ToolOutputFile 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/ToolOutputFile.h"
+#include "llvm/Support/FileSystem.h"
+#include "gtest/gtest.h"
+
+using namespace llvm;
+
+namespace {
+
+TEST(ToolOutputFileTest, DashOpensOuts) {
+ std::error_code EC;
+ EXPECT_EQ(&ToolOutputFile("-", EC, sys::fs::OF_None).os(), &outs());
+}
+
+} // namespace
diff --git a/mlir/include/mlir/Pass/PassManager.h b/mlir/include/mlir/Pass/PassManager.h
index 7b06cf0084d8..14f2da3ebf83 100644
--- a/mlir/include/mlir/Pass/PassManager.h
+++ b/mlir/include/mlir/Pass/PassManager.h
@@ -19,7 +19,8 @@
namespace llvm {
class Any;
-raw_ostream &errs();
+class raw_fd_ostream;
+raw_fd_ostream &errs();
} // end namespace llvm
namespace mlir {
More information about the Mlir-commits
mailing list