[PATCH] D38997: Support formatting formatv_objects.

[Sam McCall via Phabricator via llvm-commits]

sammccall added inline comments.


================
Comment at: include/llvm/Support/FormatVariadicDetails.h:34
 public:
-  explicit provider_format_adapter(T &&Item) : Item(Item) {}
+  explicit provider_format_adapter(T &&Item) : Item(std::forward<T>(Item)) {}
 
----------------
zturner wrote:
> This looks incorrect?  `T` is not deduced here, so `std::forward<T>(Item)` is not using a forwarding reference.  I think the code before was correct, was it causing a problem?
This is an attempt to make it possible to pass an rvalue of a move-only type to formatv, e.g. `formatv("{0}", formatv("{0}", 1))`.

Currently this doesn't work: if an rvalue Foo is passed, `build_format_adapter` infers T = Foo, and instantiates `provider_format_adapter<Foo>`, and passes a `Foo&&`.
The `Item` member has type `Foo`, and gets initialized with an **lvalue** reference (the `Item` parameter has of type `Foo&&`, but using `Item` yields an lvalue: http://www.codesynthesis.com/~boris/blog/2012/03/06/rvalue-reference-pitfalls/). Therefore this calls the copy-constructor, which is unneccesary for movable types and fails to compile for move-only types.

Adding `std::forward` here is equivalent to `std::move` if we originally passed an rvalue to `formatv`, and is no change otherwise.

> `T` is not deduced here
Right, this is a bit unusual, but I think still correct. My intuition is `build_format_adapter` deduces T such that T&& is a forwarding reference, and then passes T explicitly - T&& is still the right forwarding reference type if we want to forward a second time, because it's the same type as the first time.

If this seems confusing, we could make Item public and treat it as an aggregate: `build_format_adapter` would do provider_format_adapter<T>{std::forward<T>(Item)}. I think it amounts to the same thing.


https://reviews.llvm.org/D38997