[PATCH] D56795: [ADT] Add streaming operators for llvm::Optional

[Sam McCall via Phabricator via llvm-commits]

sammccall accepted this revision.
sammccall added inline comments.
This revision is now accepted and ready to land.


================
Comment at: include/llvm/ADT/Optional.h:336
+  else
+    OS << None;
+  return OS;
----------------
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.


================
Comment at: utils/unittest/googletest/include/gtest/internal/custom/raw-ostream.h:46
 #include "llvm/Support/raw_ostream.h"
 #include "llvm/Support/raw_os_ostream.h"
 #include <ostream>
----------------
we might need to include optional here too


================
Comment at: utils/unittest/googletest/include/gtest/internal/custom/raw-ostream.h:62-64
 // 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.
----------------
labath wrote:
> 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.
> 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.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56795/new/

https://reviews.llvm.org/D56795