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

[Pavel Labath via Phabricator via llvm-commits]

labath marked 6 inline comments as done.
labath added inline comments.


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


Repository:
  rL LLVM

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

https://reviews.llvm.org/D56795