[PATCH] D38997: Support formatting formatv_objects.

[Zachary Turner via llvm-commits]

Could you add a unit test that looks like this?

struct Foo {
int Copies = 0;
int Moves = 0;
Foo() {}
Foo(const Foo&) : Copies(1) {}
Foo(Foo&&) : Moves(1) {}


void format(raw_ostream &S, StringRef Style) {
S << Copies << Moves;
}
};

Foo f;
std::string S = formatv(“{0}”, f).str();
EXPECT_EQ(“10”, S);
S = formatv(“{0}”, Foo()).str();
EXPECT_EQ(“01”, S);
S = formatv(“{0}”,std::move(f)).str();
EXPECT_EQ(“01”, S);
On Wed, Oct 18, 2017 at 2:41 AM Sam McCall via Phabricator <
reviews at reviews.llvm.org> wrote:

> 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
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20171018/2622a4b5/attachment.html>