[PATCH] D38997: Support formatting formatv_objects.

[Zachary Turner via llvm-commits]

Lgtm
On Wed, Oct 18, 2017 at 7:00 AM Sam McCall <sammccall at google.com> wrote:

> Added the testcase, with a couple of changes:
>  - used a format_provider specialization because that's the more
> interesting case here, I think
>  - I think the *number* of copies/moves is interesting too, we'd at least
> want to notice when changing this
>
> On Wed, Oct 18, 2017 at 2:49 PM, 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);
>>
> There's no copy here, because T is Foo&, so f is passed by reference and
> provider_format_adapter<Foo&>::Item is Foo&.
>
>
>> 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/20171019/ca2d2b53/attachment.html>