This is an archive of the discontinued LLVM Phabricator instance.

Support formatting formatv_objects.
ClosedPublic

Authored by sammccall on Oct 17 2017, 4:29 AM.

Details

Summary

Support formatting formatv_objects.

While here, fix documentation about member-formatters, and attempted
perfect-forwarding (I think).

Diff Detail

Repository
rL LLVM

Event Timeline

sammccall created this revision.Oct 17 2017, 4:29 AM
zturner accepted this revision.Oct 17 2017, 1:47 PM
This revision is now accepted and ready to land.Oct 17 2017, 1:47 PM
zturner added inline comments.Oct 17 2017, 1:53 PM
include/llvm/Support/FormatVariadicDetails.h
34 ↗(On Diff #119289)

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?

sammccall added inline comments.Oct 18 2017, 2:41 AM
include/llvm/Support/FormatVariadicDetails.h
34 ↗(On Diff #119289)

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.

sammccall updated this revision to Diff 119476.Oct 18 2017, 6:56 AM

Add test counting copies and moves for movable objects.

This revision was automatically updated to reflect the committed changes.