raw_ostream: add operator<< overload for std::error_code

[labath]

Summary

The main motivation for this is unit tests, which contain a large macro
for pretty-printing std::error_code, and this macro is duplicated in
every file that needs to do this. However, the functionality may be
useful elsewhere too.

In this patch I have reimplemented the existing ASSERT_NO_ERROR macros
to reuse the new functionality, but I have kept the macro (as a
one-liner) as it is slightly more readable than ASSERT_EQ(...,
std::error_code()).

[sammccall]

I'm lacking a bit of context here.
Why do we prefer this to EXPECT_FALSE(err) /EXPECT_EQ(err, std::errc_address_in_use) with an appropriate value printer for std::error_code?

Apart from being a bit more obscure, here the Failed() matcher doesn't provide any overload to specify the code, so this api seems to encourage people not to specify the code even if they know it.

[labath]

In D65643#1611969, @sammccall wrote:

I'm lacking a bit of context here.
Why do we prefer this to EXPECT_FALSE(err) /EXPECT_EQ(err, std::errc_address_in_use) with an appropriate value printer for std::error_code?

Apart from being a bit more obscure, here the Failed() matcher doesn't provide any overload to specify the code, so this api seems to encourage people not to specify the code even if they know it.

That's a good point. There wasn't a huge deal of thought going into this. I automatically reached for the Succeded/Failed matchers because:
a) I was involved in creating them
b) it seemed nice to have a single entity which can match all kinds of errors

(a) is not much of a reason but I think (b) has some value to it. Should the need arise we can also teach it to match ErrorOr<T>, and we can also come up with a way to match error codes or categories similar to how you can now say Failed<InfoT>() to match llvm::Errors of specific types.

OTOH, I agree that it would be nice if std::error_code came out "right" no matter how it ended up being used in the assertions. Then these matchers would be a purely optional add-on that one can use when it suits him. However, I am not sure if that is actually achievable. If I know my googletest correctly, then both operator<< and the PrintTo function need to be defined in the same namespace as the object they are printing, which in this case would be std...

[labath]

Hmm... so, by piggy-backing on the StreamSwitch thingy I was able to write a operator<<(raw_ostream, std::error_code) which would print the error message. However, that only seems to work when comparing two error_codes (EXPECT_EQ and friends). When simply testing the truth value (EXPECT_FALSE), it seems to get converted to bool before it gets printed and all I get is the signature "false is not true" :P. Maybe we could adopt the convention to write EXPECT_EQ(foo(), std::error_code())? -- I find it confusing that EXPECT_FALSE expects success anyway...

[labath]

@sammccall, what do you think of the operator<<(raw_ostream&, std::error_code) idea? Is that the direction I should go in? If so, where would the implementation of that function live? (next to raw_ostream, somewhere under llvm/Testing, or under gtest/internal/custom)

[sammccall]

In D65643#1616608, @labath wrote:

@sammccall, what do you think of the operator<<(raw_ostream&, std::error_code) idea? Is that the direction I should go in? If so, where would the implementation of that function live? (next to raw_ostream, somewhere under llvm/Testing, or under gtest/internal/custom)

Sorry about taking a while to get back to this.
So I think we should have operator<< no matter what: even if we go with ASSERT_THAT(x, Succeeded()) being able to print x still improves the output.
And it'll make std::error_code work with llvm::errs() << and formatv and so on.
I think it's fine to put it in raw_ostream.h along with the others - this isn't test-specific. We might need an overload for std::errc too?

Generally, I'd rather have the "plain" styles of assertions if the readability + errors are as good, since there's less to learn.
We still have a lot of ASSERT_TRUE(X == Y) in llvm! I think it's important this stuff work as well as possible without special knowledge.
Failed() and Succeeded() seem mostly useful when wrapping values (ErrorOr --> Failed(some_error_matcher) or Succeeded(some_value_matcher)) or to deal with the terror that is llvm::Error move-semantics. But I don't feel strongly here, if you'd like to be able to use Failed() with plain std::error_code, that seems fine in Testing/Error.h, though I probably won't use it myself.

[labath]

Thanks for the feedback. I've redone the patch to add a std::error_code overload
to llvm::raw_ostream. This makes using std::error_code in tests "almost just
work", with the "almost" being there due to the ASSERT_TRUE problem.

I've dropped the matcher part of the patch because the need for it largely
disappears with this approach.

[sammccall]

Thanks! I actually like the ASSERT_NO_ERROR macros, std::error_code() meaning success is pretty obscure to write everywhere.

lib/Support/raw_ostream.cpp

143 ↗

(On Diff #214588)

Looks like typical output is system:2 address already in use

This reads a little oddly to me: the colon is easily mistaken for (english) punctuation, but that leads to misreading the text. And the category/number are first, but I think usually less relevant than the description.

Maybe consider address already in use (system:2) or so?

[labath]

In D65643#1625108, @sammccall wrote:

Thanks! I actually like the ASSERT_NO_ERROR macros, std::error_code() meaning success is pretty obscure to write everywhere.

Yeah, I see what you mean. The thing that really annoyed me was pasting the large macro everywhere, but a one-liner is not that much of a big deal.

lib/Support/raw_ostream.cpp

143 ↗

(On Diff #214588)

Done.

[labath]

Unfortunately, I've hit a snag with this implementation. It turns out changing bool(EC) to EC != std::error_code() is not NFC because one can have two "success" values that still compare as unequal (e.g. std::error_code(0, generic_category()) and std::error_code(0, system_category()). This is a problem because std::error_code() is std::error_code(0, system_category()) and there some (though few) places in llvm that do return std::error_code(errno, generic_category()), and at least some of those places are not bugs (though I think some are).

So I don't think we can go for the EXPECT_EQ(EC, std::error_code()) option. This unfortunately brings us back to square one. I think the only way to reasonably implement this is to go the matcher route, as that is the only way (that I know of) we can call operator bool to properly determine success-ness of the value, while still being able to print the exact failure message. (Though we can keep the operator<< overload, so that matching direct matching against a known error code still works.)

What do you think ?

llvm/trunk/include/llvm/Support/raw_ostream.h

public:
   }
 
   raw_ostream &operator<<(int N) {
     return this->operator<<(static_cast<long>(N));
   }
 
   raw_ostream &operator<<(double N);
 
+  raw_ostream &operator<<(std::error_code EC);
+
   /// Output \p N in hexadecimal, without any prefix or padding.
   raw_ostream &write_hex(unsigned long long N);
 
   // Change the foreground color of text.
   raw_ostream &operator<<(Colors C);
 
   /// Output a formatted UUID with dash separators.
   using uuid_t = uint8_t[16];

llvm/trunk/lib/Support/raw_ostream.cpp

raw_ostream &raw_ostream::operator<<(unsigned long long N) {
   return *this;
 }
 
 raw_ostream &raw_ostream::operator<<(long long N) {
   write_integer(*this, static_cast<int64_t>(N), 0, IntegerStyle::Integer);
   return *this;
 }
 
+raw_ostream &raw_ostream::operator<<(std::error_code EC) {
+  return *this << EC.message() << " (" << EC.category().name() << ':'
+               << EC.value() << ')';
+}
+
 raw_ostream &raw_ostream::write_hex(unsigned long long N) {
   llvm::write_hex(*this, N, HexPrintStyle::Lower);
   return *this;
 }
 
 raw_ostream &raw_ostream::operator<<(Colors C) {
   if (C == Colors::RESET)
     resetColor();

llvm/trunk/unittests/BinaryFormat/TestFileMagic.cpp

 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/Path.h"
 
 #include "gtest/gtest.h"
 
 using namespace llvm;
 namespace fs = llvm::sys::fs;
 
-#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 {                                                                     \
-  }
+#define ASSERT_NO_ERROR(x) ASSERT_EQ(x, std::error_code())
 
 class MagicTest : public testing::Test {
 protected:
   /// Unique temporary directory in which all created filesystem entities must
   /// be placed. It is removed at the end of each test (must be empty).
   SmallString<128> TestDirectory;
 
   void SetUp() override {

llvm/trunk/unittests/Support/ErrorTest.cpp

 public:
     using ErrorInfo<TestDebugError, StringError >::ErrorInfo; // inherit constructors
     TestDebugError(const Twine &S) : ErrorInfo(S, test_error_code::unspecified) {}
     static char ID;
 };
 
 class TestErrorCategory : public std::error_category {
 public:
-  const char *name() const noexcept override { return "error"; }
+  const char *name() const noexcept override { return "test_error"; }
   std::string message(int Condition) const override {
     switch (static_cast<test_error_code>(Condition)) {
     case test_error_code::unspecified:
       return "An unknown error has occurred.";
     case test_error_code::error_1:
       return "Error 1.";
     case test_error_code::error_2:
       return "Error 2.";
auto E4 = joinErrors(make_error<TestDebugError>(test_error_code::error_1,
                                                   "Detailed information"),
                        make_error<TestDebugError>(test_error_code::error_2));
   EXPECT_EQ(toString(std::move(E4))
                 .compare("Error 1. Detailed information\n"
                          "Error 2."),
             0);
 }
 
+TEST(Error, error_codeErrorMessageTest) {
+  EXPECT_NONFATAL_FAILURE(
+      EXPECT_EQ(make_error_code(test_error_code::unspecified),
+                make_error_code(test_error_code::error_2)),
+      "Which is: An unknown error has occurred. (test_error:1)");
+}
+
 } // namespace

llvm/trunk/unittests/Support/FileOutputBufferTest.cpp

 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/raw_ostream.h"
 #include "gtest/gtest.h"
 
 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 {                                                                     \
-  }
+#define ASSERT_NO_ERROR(x) ASSERT_EQ(x, std::error_code())
 
 namespace {
 TEST(FileOutputBuffer, Test) {
   // Create unique temporary directory for these tests
   SmallString<128> TestDirectory;
   {
     ASSERT_NO_ERROR(
         fs::createUniqueDirectory("FileOutputBuffer-test", TestDirectory));

llvm/trunk/unittests/Support/Host.cpp

 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/Triple.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/Program.h"
 
 #include "gtest/gtest.h"
 
-#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 {                                                                     \
-  }
+#define ASSERT_NO_ERROR(x) ASSERT_EQ(x, std::error_code())
 
 using namespace llvm;
 
 class HostTest : public testing::Test {
   Triple Host;
 
 protected:
   bool isSupportedArchAndOS() {

llvm/trunk/unittests/Support/Path.cpp

 #ifdef LLVM_ON_UNIX
 #include <pwd.h>
 #include <sys/stat.h>
 #endif
 
 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 {                                                                     \
-  }
-
-#define ASSERT_ERROR(x)                                                        \
-  if (!x) {                                                                    \
-    SmallString<128> MessageStorage;                                           \
-    raw_svector_ostream Message(MessageStorage);                               \
-    Message << #x ": did not return a failure error code.\n";                  \
-    GTEST_FATAL_FAILURE_(MessageStorage.c_str());                              \
-  }
+#define ASSERT_NO_ERROR(x) ASSERT_EQ(x, std::error_code())
+#define ASSERT_ERROR(x) ASSERT_NE(x, std::error_code())
 
 namespace {
 
 struct FileDescriptorCloser {
   explicit FileDescriptorCloser(int FD) : FD(FD) {}
   ~FileDescriptorCloser() { ::close(FD); }
   int FD;
 };
TEST_F(FileSystemTest, OpenFileForRead) {
 
   // Make sure it exists.
   ASSERT_TRUE(sys::fs::exists(Twine(TempPath)));
 
   // Open the file for read
   int FileDescriptor2;
   SmallString<64> ResultPath;
   ASSERT_NO_ERROR(fs::openFileForRead(Twine(TempPath), FileDescriptor2,
-                                      fs::OF_None, &ResultPath))
+                                      fs::OF_None, &ResultPath));
 
   // If we succeeded, check that the paths are the same (modulo case):
   if (!ResultPath.empty()) {
     // The paths returned by createTemporaryFile and getPathFromOpenFD
     // should reference the same file on disk.
     fs::UniqueID D1, D2;
     ASSERT_NO_ERROR(fs::getUniqueID(Twine(TempPath), D1));
     ASSERT_NO_ERROR(fs::getUniqueID(Twine(ResultPath), D2));

llvm/trunk/unittests/Support/ProgramTest.cpp

 #include <windows.h>
 void sleep_for(unsigned int seconds) {
   Sleep(seconds * 1000);
 }
 #else
 #error sleep_for is not implemented on your platform.
 #endif
 
-#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 {                                                                     \
-  }
+#define ASSERT_NO_ERROR(x) ASSERT_EQ(x, std::error_code())
+
 // From TestMain.cpp.
 extern const char *TestMainArgv0;
 
 namespace {
 
 using namespace llvm;
 using namespace sys;
 

llvm/trunk/unittests/Support/ReplaceFileTest.cpp

 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/Process.h"
 #include "gtest/gtest.h"
 
 using namespace llvm;
 using namespace llvm::sys;
 
-#define ASSERT_NO_ERROR(x)                                                 \
-  do {                                                                     \
-    if (std::error_code ASSERT_NO_ERROR_ec = x) {                          \
-      errs() << #x ": did not return errc::success.\n"                     \
-             << "error number: " << ASSERT_NO_ERROR_ec.value() << "\n"     \
-             << "error message: " << ASSERT_NO_ERROR_ec.message() << "\n"; \
-    }                                                                      \
-  } while (false)
+#define ASSERT_NO_ERROR(x) ASSERT_EQ(x, std::error_code())
 
 namespace {
 std::error_code CreateFileWithContent(const SmallString<128> &FilePath,
                                       const StringRef &content) {
   int FD = 0;
   if (std::error_code ec = fs::openFileForWrite(FilePath, FD))
     return ec;
 

llvm/trunk/unittests/Support/raw_pwrite_stream_test.cpp

 #include "llvm/Config/llvm-config.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/FileUtilities.h"
 #include "llvm/Support/raw_ostream.h"
 #include "gtest/gtest.h"
 
 using namespace llvm;
 
-#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 {                                                                     \
-  }
+#define ASSERT_NO_ERROR(x) ASSERT_EQ(x, std::error_code())
 
 namespace {
 
 TEST(raw_pwrite_ostreamTest, TestSVector) {
   SmallVector<char, 0> Buffer;
   raw_svector_ostream OS(Buffer);
   OS << "abcd";
   StringRef Test = "test";