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

[Sam McCall via Phabricator via llvm-commits]

sammccall added inline comments.


================
Comment at: include/llvm/ADT/Optional.h:336
+  else
+    OS << None;
+  return OS;
----------------
labath wrote:
> sammccall wrote:
> > 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.
> 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.
Right, of course :-)


================
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:
> sammccall wrote:
> > 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.
> 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.
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 :-\


Repository:
  rL LLVM

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

https://reviews.llvm.org/D56795