From 38a4f3bbec129553e4fd91ad3a69e9bbbbfa1ddf Mon Sep 17 00:00:00 2001
From: David Blaikie <dblaikie@gmail.com>
Date: Wed, 3 Sep 2014 17:31:25 +0000
Subject: [PATCH] Ensure ErrorOr cannot implicitly invoke explicit ctors of the
 underlying type.

An unpleasant surprise while migrating unique_ptrs (see changes in
lib/Object): ErrorOr<int*> was implicitly convertible to
ErrorOr<std::unique_ptr<int>>.

Keep the explicit conversions otherwise it's a pain to convert
ErrorOr<int*> to ErrorOr<std::unique_ptr<int>>.

I'm not sure if there should be more SFINAE on those explicit ctors (I
could check if !is_convertible && is_constructible, but since the ctor
has to be called explicitly I don't think there's any need to disable
them when !is_constructible - they'll just fail anyway. It's the
converting ctors that can create interesting ambiguities without proper
SFINAE). I had to SFINAE the explicit ones because otherwise they'd be
ambiguous with the implicit ones in an explicit context, so far as I
could tell.

The converting assignment operators seemed unnecessary (and similarly
buggy/dangerous) - just rely on the converting ctors to convert to the
right type for assignment instead.

git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@217048 91177308-0d34-0410-b5e6-96231b3b80d8
---
 include/llvm/Support/ErrorOr.h    | 40 ++++++++++++++++++++-----------
 lib/Object/Binary.cpp             |  3 ++-
 lib/Object/SymbolicFile.cpp       |  3 ++-
 unittests/Support/ErrorOrTest.cpp | 31 ++++++++++++++++++++++++
 4 files changed, 61 insertions(+), 16 deletions(-)

diff --git a/include/llvm/Support/ErrorOr.h b/include/llvm/Support/ErrorOr.h
index 6f164805b5c..68ab94609c1 100644
--- a/include/llvm/Support/ErrorOr.h
+++ b/include/llvm/Support/ErrorOr.h
@@ -115,19 +115,19 @@ public:
   }
 
   template <class OtherT>
-  ErrorOr(const ErrorOr<OtherT> &Other) {
+  ErrorOr(
+      const ErrorOr<OtherT> &Other,
+      typename std::enable_if<std::is_convertible<OtherT, T>::value>::type * =
+          nullptr) {
     copyConstruct(Other);
   }
 
-  ErrorOr &operator =(const ErrorOr &Other) {
-    copyAssign(Other);
-    return *this;
-  }
-
   template <class OtherT>
-  ErrorOr &operator =(const ErrorOr<OtherT> &Other) {
-    copyAssign(Other);
-    return *this;
+  explicit ErrorOr(
+      const ErrorOr<OtherT> &Other,
+      typename std::enable_if<
+          !std::is_convertible<OtherT, const T &>::value>::type * = nullptr) {
+    copyConstruct(Other);
   }
 
   ErrorOr(ErrorOr &&Other) {
@@ -135,17 +135,29 @@ public:
   }
 
   template <class OtherT>
-  ErrorOr(ErrorOr<OtherT> &&Other) {
+  ErrorOr(
+      ErrorOr<OtherT> &&Other,
+      typename std::enable_if<std::is_convertible<OtherT, T>::value>::type * =
+          nullptr) {
     moveConstruct(std::move(Other));
   }
 
-  ErrorOr &operator =(ErrorOr &&Other) {
-    moveAssign(std::move(Other));
+  // This might eventually need SFINAE but it's more complex than is_convertible
+  // & I'm too lazy to write it right now.
+  template <class OtherT>
+  explicit ErrorOr(
+      ErrorOr<OtherT> &&Other,
+      typename std::enable_if<!std::is_convertible<OtherT, T>::value>::type * =
+          nullptr) {
+    moveConstruct(std::move(Other));
+  }
+
+  ErrorOr &operator=(const ErrorOr &Other) {
+    copyAssign(Other);
     return *this;
   }
 
-  template <class OtherT>
-  ErrorOr &operator =(ErrorOr<OtherT> &&Other) {
+  ErrorOr &operator=(ErrorOr &&Other) {
     moveAssign(std::move(Other));
     return *this;
   }
diff --git a/lib/Object/Binary.cpp b/lib/Object/Binary.cpp
index d23ee590569..d9fef8be8e1 100644
--- a/lib/Object/Binary.cpp
+++ b/lib/Object/Binary.cpp
@@ -63,7 +63,8 @@ ErrorOr<std::unique_ptr<Binary>> object::createBinary(MemoryBufferRef Buffer,
     case sys::fs::file_magic::bitcode:
       return ObjectFile::createSymbolicFile(Buffer, Type, Context);
     case sys::fs::file_magic::macho_universal_binary:
-      return MachOUniversalBinary::create(Buffer);
+      return ErrorOr<std::unique_ptr<Binary>>(
+          MachOUniversalBinary::create(Buffer));
     case sys::fs::file_magic::unknown:
     case sys::fs::file_magic::windows_resource:
       // Unrecognized object file format.
diff --git a/lib/Object/SymbolicFile.cpp b/lib/Object/SymbolicFile.cpp
index f8dd4b33a39..17624a307ec 100644
--- a/lib/Object/SymbolicFile.cpp
+++ b/lib/Object/SymbolicFile.cpp
@@ -33,7 +33,8 @@ ErrorOr<std::unique_ptr<SymbolicFile>> SymbolicFile::createSymbolicFile(
   switch (Type) {
   case sys::fs::file_magic::bitcode:
     if (Context)
-      return IRObjectFile::createIRObjectFile(Object, *Context);
+      return ErrorOr<std::unique_ptr<SymbolicFile>>(
+          IRObjectFile::createIRObjectFile(Object, *Context));
   // Fallthrough
   case sys::fs::file_magic::unknown:
   case sys::fs::file_magic::archive:
diff --git a/unittests/Support/ErrorOrTest.cpp b/unittests/Support/ErrorOrTest.cpp
index d76e7d62421..82bbe090960 100644
--- a/unittests/Support/ErrorOrTest.cpp
+++ b/unittests/Support/ErrorOrTest.cpp
@@ -60,5 +60,36 @@ TEST(ErrorOr, Covariant) {
 
   ErrorOr<std::unique_ptr<B> > b1(ErrorOr<std::unique_ptr<D> >(nullptr));
   b1 = ErrorOr<std::unique_ptr<D> >(nullptr);
+
+  ErrorOr<std::unique_ptr<int>> b2(ErrorOr<int *>(nullptr));
+  ErrorOr<int *> b3(nullptr);
+  ErrorOr<std::unique_ptr<int>> b4(b3);
 }
+
+// ErrorOr<int*> x(nullptr);
+// ErrorOr<std::unique_ptr<int>> y = x; // invalid conversion
+static_assert(
+    !std::is_convertible<const ErrorOr<int *> &,
+                         ErrorOr<std::unique_ptr<int>>>::value,
+    "do not invoke explicit ctors in implicit conversion from lvalue");
+
+// ErrorOr<std::unique_ptr<int>> y = ErrorOr<int*>(nullptr); // invalid
+//                                                           // conversion
+static_assert(
+    !std::is_convertible<ErrorOr<int *> &&,
+                         ErrorOr<std::unique_ptr<int>>>::value,
+    "do not invoke explicit ctors in implicit conversion from rvalue");
+
+// ErrorOr<int*> x(nullptr);
+// ErrorOr<std::unique_ptr<int>> y;
+// y = x; // invalid conversion
+static_assert(!std::is_assignable<ErrorOr<std::unique_ptr<int>>,
+                                  const ErrorOr<int *> &>::value,
+              "do not invoke explicit ctors in assignment");
+
+// ErrorOr<std::unique_ptr<int>> x;
+// x = ErrorOr<int*>(nullptr); // invalid conversion
+static_assert(!std::is_assignable<ErrorOr<std::unique_ptr<int>>,
+                                  ErrorOr<int *> &&>::value,
+              "do not invoke explicit ctors in assignment");
 } // end anon namespace
-- 
2.40.0