[PATCH] D49170: [Support] Require llvm::Error passed to formatv() to be wrapped in fmt_consume()

[Sam McCall via Phabricator via llvm-commits]

sammccall created this revision.
sammccall added a reviewer: zturner.
Herald added a subscriber: llvm-commits.

Someone must be responsible for handling an Error. When formatv takes
ownership of an Error, the formatv_object destructor must take care of this.

Passing an error by value to formatv() is not considered explicit enough to mark
the error as handled (see https://reviews.llvm.org/D49013), so we require callers to use a format adapter
to confirm this intent.


Repository:
  rL LLVM

https://reviews.llvm.org/D49170

Files:
  include/llvm/Support/FormatAdapters.h
  include/llvm/Support/FormatVariadicDetails.h
  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}", fmt_consume(std::move(E1))).str());
+  EXPECT_FALSE(E1.isA<StringError>()); // consumed
+}
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 {
@@ -141,6 +142,12 @@
 typename std::enable_if<uses_stream_operator<T>::value,
                         stream_operator_format_adapter<T>>::type
 build_format_adapter(T &&Item) {
+  // If the caller passed an Error by value, then stream_operator_format_adapter
+  // would be responsible for consuming it.
+  // Make the caller opt into this by calling fmt_consume().
+  static_assert(
+      !std::is_same<llvm::Error, typename std::remove_cv<T>::type>::value,
+      "llvm::Error-by-value must be wrapped in fmt_consume() for formatv");
   return stream_operator_format_adapter<T>(std::forward<T>(Item));
 }
 
Index: include/llvm/Support/FormatAdapters.h
===================================================================
--- include/llvm/Support/FormatAdapters.h
+++ include/llvm/Support/FormatAdapters.h
@@ -12,14 +12,15 @@
 
 #include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/StringRef.h"
+#include "llvm/Support/Error.h"
 #include "llvm/Support/FormatCommon.h"
 #include "llvm/Support/FormatVariadicDetails.h"
 #include "llvm/Support/raw_ostream.h"
 
 namespace llvm {
 template <typename T> class FormatAdapter : public detail::format_adapter {
 protected:
-  explicit FormatAdapter(T &&Item) : Item(Item) {}
+  explicit FormatAdapter(T &&Item) : Item(std::forward<T>(Item)) {}
 
   T Item;
 };
@@ -71,6 +72,14 @@
     }
   }
 };
+
+class ErrorAdapter : public FormatAdapter<Error> {
+public:
+  ErrorAdapter(Error &&Item) : FormatAdapter(std::move(Item)) {}
+  ErrorAdapter(ErrorAdapter &&) = default;
+  ~ErrorAdapter() { consumeError(std::move(Item)); }
+  void format(llvm::raw_ostream &Stream, StringRef Style) { Stream << Item; }
+};
 }
 
 template <typename T>
@@ -88,6 +97,13 @@
 detail::RepeatAdapter<T> fmt_repeat(T &&Item, size_t Count) {
   return detail::RepeatAdapter<T>(std::forward<T>(Item), Count);
 }
+
+// llvm::Error values must be consumed before being destroyed.
+// Wrapping an error in fmt_consume explicitly indicates that the formatv_object
+// should take ownership and consume it.
+detail::ErrorAdapter fmt_consume(Error &&Item) {
+  return detail::ErrorAdapter(std::move(Item));
+}
 }
 
 #endif


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D49170.154942.patch
Type: text/x-patch
Size: 3393 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20180711/d79168d1/attachment.bin>