Maybe two more line at the bottom that says <br><br>EXPECT_EQ(0, f.Copies);<br>EXPECT_EQ(0, f.Moves);<br><br>Should be obvious, but this makes it a bit more self documenting <br><div class="gmail_quote"><div dir="ltr">On Wed, Oct 18, 2017 at 5:49 AM Zachary Turner <<a href="mailto:zturner@google.com">zturner@google.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Could you add a unit test that looks like this?<br><br>struct Foo {<br>  int Copies = 0;<br>  int Moves = 0;<br>  Foo() {}<br>  Foo(const Foo&) : Copies(1) {}<br>  Foo(Foo&&) : Moves(1) {}<br><br><br>  void format(raw_ostream &S, StringRef Style) {<br>    S << Copies << Moves;<br>  }<br>};<br><br>Foo f;<br>std::string S = formatv(“{0}”, f).str();<br>EXPECT_EQ(“10”, S);<br>S = formatv(“{0}”, Foo()).str();<br>EXPECT_EQ(“01”, S);<br>S = formatv(“{0}”,std::move(f)).str();<br>EXPECT_EQ(“01”, S);<br><div class="gmail_quote"><div dir="ltr">On Wed, Oct 18, 2017 at 2:41 AM Sam McCall via Phabricator <<a href="mailto:reviews@reviews.llvm.org" target="_blank">reviews@reviews.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">sammccall added inline comments.<br>
<br>
<br>
================<br>
Comment at: include/llvm/Support/FormatVariadicDetails.h:34<br>
 public:<br>
-  explicit provider_format_adapter(T &&Item) : Item(Item) {}<br>
+  explicit provider_format_adapter(T &&Item) : Item(std::forward<T>(Item)) {}<br>
<br>
----------------<br>
zturner wrote:<br>
> 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?<br>
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))`.<br>
<br>
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&&`.<br>
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: <a href="http://www.codesynthesis.com/~boris/blog/2012/03/06/rvalue-reference-pitfalls/" rel="noreferrer" target="_blank">http://www.codesynthesis.com/~boris/blog/2012/03/06/rvalue-reference-pitfalls/</a>). Therefore this calls the copy-constructor, which is unneccesary for movable types and fails to compile for move-only types.<br>
<br>
Adding `std::forward` here is equivalent to `std::move` if we originally passed an rvalue to `formatv`, and is no change otherwise.<br>
<br>
> `T` is not deduced here<br>
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.<br>
<br>
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.<br>
<br>
<br>
<a href="https://reviews.llvm.org/D38997" rel="noreferrer" target="_blank">https://reviews.llvm.org/D38997</a><br>
<br>
<br>
<br>
</blockquote></div></blockquote></div>