<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Nov 18, 2015 at 11:06 AM, Sanjoy Das <span dir="ltr"><<a href="mailto:sanjoy@playingwithpointers.com" target="_blank">sanjoy@playingwithpointers.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class=""><br>
<br>
David Blaikie wrote:<br>
>     -template <typename InputTy> struct OperandBundleDefT {<br>
>     +template <typename InputTy> class OperandBundleDefT {<br>
>         std::string Tag;<br>
>         std::vector<InputTy> Inputs;<br>
><br>
>     -  OperandBundleDefT() {}<br>
>     -  explicit OperandBundleDefT(StringRef Tag, const<br>
>     std::vector<InputTy> &Inputs)<br>
>     +public:<br>
>     +  explicit OperandBundleDefT(StringRef Tag, std::vector<InputTy><br>
>     &&Inputs)<br>
><br>
><br>
> As a general rule, you shouldn't pass by rvalue reference - instead pass<br>
> by value (that way you don't force callers to provide an rvalue when<br>
> they may not want to - but if they do provide one, it's efficiently<br>
> moved, etc) - similar below<br>
><br>
>             : Tag(Tag), Inputs(Inputs) {}<br>
><br>
><br>
> Without the std::move here ^ ("Inputs(std::move(Inputs))") this will be<br>
> a copy where a move was probably intended (similarly below)<br>
<br></span>
Just to be clear, you're suggesting changing this to:<br>
<br>
 struct Foo {<br>
  explicit Foo(std::vector<V> vect) member(std::move(vect)) {}<br></blockquote><div><br></div><div>Yep, just like that</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
 }<br>
<br>
If so, then that sounds like a good idea.<span class=""><br>
<br>
>     -  explicit OperandBundleDefT(StringRef Tag, std::vector<InputTy><br>
>     &&Inputs)<br>
>     +  explicit OperandBundleDefT(std::string &&Tag,<br>
>     std::vector<InputTy> &&Inputs)<br>
>             : Tag(Tag), Inputs(Inputs) {}<br>
><br>
>         explicit OperandBundleDefT(const OperandBundleUse &OBU) {<br>
>           Tag = OBU.getTagName();<br>
>           Inputs.insert(Inputs.end(), OBU.Inputs.begin(), OBU.Inputs.end());<br>
>         }<br>
>     +<br>
>     +  ArrayRef<InputTy> getInputs() const { return Inputs; }<br>
><br>
><br>
> Probably just name this ^ "inputs()" to be consistent with other range<br>
> accessors?<br>
<br></span>
Yeah, that sounds like a good idea -- will make it inconsistent with<br>
args(), arg_size(), arg_begin(), arg_end() etc.<span class="HOEnZb"><font color="#888888"><br>
<br>
-- Sanjoy<br>
</font></span></blockquote></div><br></div></div>