[PATCH] D38997: Support formatting formatv_objects.

[Zachary Turner via llvm-commits]

Maybe two more line at the bottom that says

EXPECT_EQ(0, f.Copies);
EXPECT_EQ(0, f.Moves);

Should be obvious, but this makes it a bit more self documenting
On Wed, Oct 18, 2017 at 5:49 AM Zachary Turner <zturner at google.com> wrote:

> 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/f993d643/attachment.html>