[ADT] Add streaming operators for llvm::Optional

[labath]

Summary

The operators simply print the underlying value or "None".

The trickier part of this patch is making sure the streaming operators
work even in unit tests (which was my primary motivation, though I can
also see them being useful elsewhere). Since the stream operator was a
template, implicit conversions did not kick in, and our gtest glue code
was explicitly introducing an implicit conversion to make sure other
implicit conversions do not kick in :P. I resolve that by specializing
llvm_gtest::StreamSwitch for llvm:Optional<T>.

[labath]

utils/unittest/googletest/include/gtest/internal/custom/raw-ostream.h

62–64 ↗

(On Diff #182081)

It's not clear to me how much of an issue are these implicit conversion. I would expect that if a type has an implicit conversion to bool, then this conversion would kick in also during normal streaming usage. (?) If that's the case, then maybe that's something that the type's author should fix (by making the conversion explicit?) instead of trying to work around it here.

[sammccall]

include/llvm/ADT/Optional.h

336 ↗

(On Diff #182081)

alternatively could just OS << "None" here and skip the NoneType version (and thus the cpp file) altogether - I'm not sure being able to print NoneType adds any value. Up to you though.

utils/unittest/googletest/include/gtest/internal/custom/raw-ostream.h

46 ↗

(On Diff #182081)

we might need to include optional here too

62–64 ↗

(On Diff #182081)

I would expect that if a type has an implicit conversion to bool, then this conversion would kick in also during normal streaming usage. (?)

In normal streaming usage, the conversion would kick in only if required.

Example: suppose <<(std::stream, int), <<(std::stream, bool), <<(raw_ostream, bool), and implicit int -> bool all exist.

• In normal streaming usage, stream() << 42 resolves to <<(stream, int) rather than <<(stream, bool) because no conversion is required.
• If we don't suppress conversions here, stream() << printable(42) resolves to <<(stream, RawStreamProxy<int>), whose body calls raw_os_ostream(S) << 42, which resolves to <<(raw_ostream, bool)
• If we suppress conversions here, StreamSwitch<int> doesn't get specialized, because raw_ostream << ConvertibleTo<int> can't compile. Therefore printable(42) is just 42, and the call resolves to <<(stream, int) as before.

I definitely saw this problem in test failure messages, but I didn't actually add a test, and can't remember what the actual problematic types were :-(

If there was a metaprogramming way to perform lookup/overload resolution with T, and then check whether you can apply the resulting operator<< to ConvertibleTo<T>, then this would be easy enough to solve.

[labath]

include/llvm/ADT/Optional.h

336 ↗

(On Diff #182081)

It slightly improves the output of EXPECT_EQ/NE(None, whatever()). Without it, it would still be understandable because the textual version of LHS would contain "None", but we would also get the "Which is: 1-byte object ..." blurb after it.

utils/unittest/googletest/include/gtest/internal/custom/raw-ostream.h

62–64 ↗

(On Diff #182081)

I see the issue now. Thanks for the explanation. It's too bad you don't remember the types - I'd be interested to see them because it sounds like it would have to have a very specific set of operators defined for this to be a problem: IIUC, it would have to have an exact std::ostream<< operator, which would actually be better than a llvm::raw_ostream<< operator for a type to which it can be implicitly converted to.

I don't think there's a way to implement overload resolution independently of actually calling the operator, but what I could do is change this logic to consider the operators in the following order of preference:

1. <<(llvm::raw_ostream, T) without any conversions
2. <<(std::ostream, T) without any conversions
3. <<(llvm::raw_ostream, T) with implicit conversions
4. everything else (including the default gtest printer)

However, I'm not sure if that's actually better than specializing for Optional (and possibly other types in the future). LMK, if you want me to implement that instead.

[sammccall]

include/llvm/ADT/Optional.h

336 ↗

(On Diff #182081)

Right, of course :-)

utils/unittest/googletest/include/gtest/internal/custom/raw-ostream.h

62–64 ↗

(On Diff #182081)

Yeah, I really wish I could remember what I ran into (or better, had written a test with a useful comment). Maybe it was raw pointers?

That sequence seems like it would solve the problem, though I worry that allowing implicit conversions for raw_ostream might override some of the good defaults provided by gtest (again, maybe in the raw pointer case).

I'd probably keep it like this with the special case until it turns out to be a bigger problem, though :-\

[labath]

utils/unittest/googletest/include/gtest/internal/custom/raw-ostream.h

62–64 ↗

(On Diff #182081)

Yes, it sounds to me like this can only be an issue for some builtin/standard library stuff. Llvm types are not going to have std::ostream operators defined.

Ok, I'll commit this as is (with the #include <Optional> added). If we need to revisit this in the future, maybe the right solution would be to explicitly "despecialize" StreamSwitch for raw pointers and any other type deemed necessary.

llvm/trunk/include/llvm/ADT/Optional.h

 #include "llvm/Support/type_traits.h"
 #include <algorithm>
 #include <cassert>
 #include <new>
 #include <utility>
 
 namespace llvm {
 
+class raw_ostream;
+
 namespace optional_detail {
 /// Storage for any type.
 template <typename T, bool = isPodLike<T>::value> struct OptionalStorage {
   AlignedCharArrayUnion<T> storage;
   bool hasVal = false;
 
   OptionalStorage() = default;
 

 template <typename T> bool operator>=(const Optional<T> &X, const T &Y) {
   return !(X < Y);
 }
 
 template <typename T> bool operator>=(const T &X, const Optional<T> &Y) {
   return !(X < Y);
 }
 
+raw_ostream &operator<<(raw_ostream &OS, NoneType);
+
+template <typename T, typename = decltype(std::declval<raw_ostream &>()
+                                          << std::declval<const T &>())>
+raw_ostream &operator<<(raw_ostream &OS, const Optional<T> &O) {
+  if (O)
+    OS << *O;
+  else
+    OS << None;
+  return OS;
+}
+
 } // end namespace llvm
 
 #endif // LLVM_ADT_OPTIONAL_H

llvm/trunk/lib/Support/CMakeLists.txt

add_llvm_library(LLVMSupport
   Locale.cpp
   LockFileManager.cpp
   LowLevelType.cpp
   ManagedStatic.cpp
   MathExtras.cpp
   MemoryBuffer.cpp
   MD5.cpp
   NativeFormatting.cpp
+  Optional.cpp
   Options.cpp
   Parallel.cpp
   PluginLoader.cpp
   PrettyStackTrace.cpp
   RandomNumberGenerator.cpp
   Regex.cpp
   ScaledNumber.cpp
   ScopedPrinter.cpp

llvm/trunk/lib/Support/Optional.cpp

+//===- Optional.cpp - Optional values ---------------------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "llvm/ADT/Optional.h"
+#include "llvm/Support/raw_ostream.h"
+
+llvm::raw_ostream &llvm::operator<<(raw_ostream &OS, NoneType) {
+  return OS << "None";
+}

llvm/trunk/unittests/ADT/OptionalTest.cpp

 //===- llvm/unittest/ADT/OptionalTest.cpp - Optional unit tests -----------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "llvm/ADT/Optional.h"
+#include "llvm/ADT/SmallString.h"
+#include "llvm/Support/raw_ostream.h"
+#include "gtest/gtest-spi.h"
 #include "gtest/gtest.h"
 
 using namespace llvm;
 
 namespace {
 
 struct NonDefaultConstructible {
   static unsigned CopyConstructions;
TEST_F(OptionalTest, OperatorGreaterEqual) {
   CheckRelation<GreaterEqual>(InequalityLhs, NoneIneq, true);
 
   IsLess = false;
   CheckRelation<GreaterEqual>(InequalityLhs, InequalityRhs, !IsLess);
   IsLess = true;
   CheckRelation<GreaterEqual>(InequalityLhs, InequalityRhs, !IsLess);
 }
 
-} // end anonymous namespace
+struct ComparableAndStreamable {
+  friend bool operator==(ComparableAndStreamable,
+                         ComparableAndStreamable) LLVM_ATTRIBUTE_USED {
+    return true;
+  }
 
+  friend raw_ostream &operator<<(raw_ostream &OS, ComparableAndStreamable) {
+    return OS << "ComparableAndStreamable";
+  }
+
+  static Optional<ComparableAndStreamable> get() {
+    return ComparableAndStreamable();
+  }
+};
+
+TEST_F(OptionalTest, StreamOperator) {
+  auto to_string = [](Optional<ComparableAndStreamable> O) {
+    SmallString<16> S;
+    raw_svector_ostream OS(S);
+    OS << O;
+    return S;
+  };
+  EXPECT_EQ("ComparableAndStreamable",
+            to_string(ComparableAndStreamable::get()));
+  EXPECT_EQ("None", to_string(None));
+}
+
+struct Comparable {
+  friend bool operator==(Comparable, Comparable) LLVM_ATTRIBUTE_USED {
+    return true;
+  }
+  static Optional<Comparable> get() { return Comparable(); }
+};
+
+TEST_F(OptionalTest, UseInUnitTests) {
+  // Test that we invoke the streaming operators when pretty-printing values in
+  // EXPECT macros.
+  EXPECT_NONFATAL_FAILURE(EXPECT_EQ(llvm::None, ComparableAndStreamable::get()),
+                          R"(Expected: llvm::None
+      Which is: None
+To be equal to: ComparableAndStreamable::get()
+      Which is: ComparableAndStreamable)");
+
+  // Test that it is still possible to compare objects which do not have a
+  // custom streaming operator.
+  EXPECT_NONFATAL_FAILURE(EXPECT_EQ(llvm::None, Comparable::get()), "object");
+}
+
+} // end anonymous namespace

llvm/trunk/utils/unittest/googletest/include/gtest/internal/custom/raw-ostream.h

auto printable(const T &V) -> decltype(StreamSwitch<T>::printable(V)) {
   // We delegate to the trait, to allow partial specialization.
   return StreamSwitch<T>::printable(V);
 }
 } // namespace llvm_gtest
 
 // If raw_ostream support is enabled, we specialize for types with operator<<
 // that takes a raw_ostream.
 #if !GTEST_NO_LLVM_RAW_OSTREAM
-#include "llvm/Support/raw_ostream.h"
+#include "llvm/ADT/Optional.h"
 #include "llvm/Support/raw_os_ostream.h"
+#include "llvm/Support/raw_ostream.h"
 #include <ostream>
 namespace llvm_gtest {
 
 // The printable() of a raw_ostream-enabled type T is a RawStreamProxy<T>.
 // It uses raw_os_ostream to write the wrapped value to a std::ostream.
 template <typename T>
 struct RawStreamProxy {
   const T& V;
   friend std::ostream &operator<<(std::ostream &S, const RawStreamProxy<T> &V) {
     llvm::raw_os_ostream OS(S);
     OS << V.V;
     return S;
   }
 };
 
 // We enable raw_ostream treatment if `(raw_ostream&) << (const T&)` is valid.
 // We don't want implicit conversions on the RHS (e.g. to bool!), so "consume"
 // the possible conversion by passing something convertible to const T& instead.
 template <typename T> struct ConvertibleTo { operator T(); };
 template <typename T>
 struct StreamSwitch<T, decltype((void)(std::declval<llvm::raw_ostream &>()
                                        << ConvertibleTo<const T &>()))> {
   static const RawStreamProxy<T> printable(const T &V) { return {V}; }
 };
+
+// llvm::Optional has a template operator<<, which means it will not accept any
+// implicit conversions, so we need to special-case it here.
+template <typename T>
+struct StreamSwitch<llvm::Optional<T>,
+                    decltype((void)(std::declval<llvm::raw_ostream &>()
+                                    << std::declval<llvm::Optional<T>>()))> {
+  static const RawStreamProxy<llvm::Optional<T>>
+  printable(const llvm::Optional<T> &V) {
+    return {V};
+  }
+};
 } // namespace llvm_gtest
 #endif  // !GTEST_NO_LLVM_RAW_OSTREAM
 
 #endif // GTEST_INCLUDE_GTEST_INTERNAL_CUSTOM_RAW_OSTREAM_H_