[PATCH] D49013: [Support] Allow formatv() to consume llvm::Error by-value without crashing.

[Sam McCall via Phabricator via llvm-commits]

sammccall created this revision.
sammccall added reviewers: zturner, ioeric.
Herald added a subscriber: llvm-commits.

Currently the following code crashes if an error occurs:

  llvm::Expected<Foo> MaybeFoo = ...
  if (!MaybeFoo)
    llvm::errs() << formatv("Error: {0}", MaybeFoo.takeError());

I feel conflicted about adding a special case like this, but

- Error has an unusual API that interacts strangely with other libraries.
- it's fairly important, and
- it gets used conditionally on error-handling paths that are rarely tested.

The main alternative I can see is to remove operator<< from Error again.
This requires you to write formatv(..., llvm::toString(MaybeFoo.takeError())),
and provides no way of printing an error by reference.


Repository:
  rL LLVM

https://reviews.llvm.org/D49013

Files:
  include/llvm/Support/FormatVariadicDetails.h
  lib/Support/FormatVariadic.cpp
  unittests/Support/FormatVariadicTest.cpp


Index: unittests/Support/FormatVariadicTest.cpp
===================================================================
--- unittests/Support/FormatVariadicTest.cpp
+++ unittests/Support/FormatVariadicTest.cpp
@@ -8,6 +8,7 @@
 //===----------------------------------------------------------------------===//
 
 #include "llvm/Support/FormatVariadic.h"
+#include "llvm/Support/Error.h"
 #include "llvm/Support/FormatAdapters.h"
 #include "gtest/gtest.h"
 
@@ -680,3 +681,11 @@
   adl::X X;
   EXPECT_EQ("X", formatv("{0}", X).str());
 }
+
+TEST(FormatVariadicTest, FormatError) {
+  auto E1 = make_error<StringError>("X", inconvertibleErrorCode());
+  EXPECT_EQ("X", formatv("{0}", E1).str());
+  EXPECT_TRUE(E1.isA<StringError>()); // not consumed
+  EXPECT_EQ("X", formatv("{0}", std::move(E1)).str());
+  EXPECT_FALSE(E1.isA<StringError>()); // consumed
+}
Index: lib/Support/FormatVariadic.cpp
===================================================================
--- lib/Support/FormatVariadic.cpp
+++ lib/Support/FormatVariadic.cpp
@@ -7,6 +7,7 @@
 //===----------------------------------------------------------------------===//
 
 #include "llvm/Support/FormatVariadic.h"
+#include "llvm/Support/Error.h"
 
 using namespace llvm;
 
@@ -152,3 +153,5 @@
   }
   return Replacements;
 }
+
+void detail::cleanup(llvm::Error &&E) { consumeError(std::move(E)); }
Index: include/llvm/Support/FormatVariadicDetails.h
===================================================================
--- include/llvm/Support/FormatVariadicDetails.h
+++ include/llvm/Support/FormatVariadicDetails.h
@@ -17,6 +17,7 @@
 
 namespace llvm {
 template <typename T, typename Enable = void> struct format_provider {};
+class Error;
 
 namespace detail {
 class format_adapter {
@@ -38,15 +39,22 @@
   }
 };
 
+// Hack to allow accepting Error by value: formatv("Error: {0}", V.takeError());
+// In this case the error must be marked as consumed.
+template <typename T> void cleanup(T &&) {}
+void cleanup(llvm::Error &&E);
+
 template <typename T>
 class stream_operator_format_adapter : public format_adapter {
   T Item;
 
 public:
   explicit stream_operator_format_adapter(T &&Item)
       : Item(std::forward<T>(Item)) {}
+  stream_operator_format_adapter(stream_operator_format_adapter &&) = default;
 
   void format(llvm::raw_ostream &S, StringRef Options) override { S << Item; }
+  ~stream_operator_format_adapter() { detail::cleanup(std::forward<T>(Item)); }
 };
 
 template <typename T> class missing_format_adapter;


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D49013.154378.patch
Type: text/x-patch
Size: 2499 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20180706/029bee41/attachment.bin>