<div dir="ltr"><br><br><div class="gmail_quote"><div dir="ltr">On Fri, Sep 2, 2016 at 10:57 AM Aaron Ballman <<a href="mailto:aaron.ballman@gmail.com">aaron.ballman@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">aaron.ballman added inline comments.<br>
<br>
================<br>
Comment at: lib/Sema/SemaOverload.cpp:12996<br>
@@ +12995,3 @@<br>
+      // selection expression.<br>
+      std::vector<Expr *> AssocExprs(GSE->getAssocExprs().vec());<br>
+      unsigned ResultIdx = GSE->getResultIndex();<br>
----------------<br>
george.burgess.iv wrote:<br>
> aaron.ballman wrote:<br>
> > dblaikie wrote:<br>
> > > george.burgess.iv wrote:<br>
> > > > Is there a reason this isn't a `SmallVector` instead?<br>
> > > Another note on this - we should generally prefer copy init over direct init (less power, less responsibility/easier to read):<br>
> > ><br>
> > >   std::vector<Expr*> AssocExprs = GSE->getAssocExprs().vec();<br>
> > ><br>
> > > (& as for George's question: since ArrayRef::vec returns std::vector, it's cheaper to store in a std::vector (by move) than to make a copy into a SmallVector)<br>
> > Yeah, I originally used std::vector<> because of ArrayRef's interface. I am happy to go either route, depending on preference, as I doubt this will wind up on the hot path with any regularity.<br>
> > as for George's question: since ArrayRef::vec returns std::vector, it's cheaper to store in a std::vector (by move) than to make a copy into a SmallVector<br>
><br>
> I was thinking that we would end up using the `SmallVector(begin(), end())` ctor instead, so the vector temp wouldn't be needed. :)<br>
><br>
> Regardless, it was just a nit, so I'm perfectly happy if it stays a `vector`.<br>
><br>
> > I doubt this will wind up on the hot path with any regularity<br>
><br>
> Agreed.<br>
> I was thinking that we would end up using the SmallVector(begin(), end()) ctor instead, so the vector temp wouldn't be needed. :)<br>
<br>
I went that route, but didn't like requiring a local `ArrayRef<Expr *>` to prevent calling `GSE->getAssocExprs()` twice. There really is no convenient way to get a SmallVector from an ArrayRef, so perhaps another option would be to have a SmallVector constructor that accepts an ArrayRef, or have a way to get all of the elements as an `llvm::iterator_range` directly from an ArrayRef.<br></blockquote><div><br></div><div>Yeah, we really should (if possible) change this ctor: <a href="http://llvm.org/docs/doxygen/html/classllvm_1_1SmallVector.html#a12de5fa9d857aa18e9edac9db6a79838">http://llvm.org/docs/doxygen/html/classllvm_1_1SmallVector.html#a12de5fa9d857aa18e9edac9db6a79838</a> - to take an opaque range, any range, not just an iterator_range. Then you could construct a SmallVector from an ArrayRef.<br><br>(would have to SFINAE on the existence of ADL-available begin/end free functions and an element type implicitly convertible to the SmallVector's element type probably, <span style="line-height:1.5">to make sure the ctor wasn't firing on any/all arguments)</span></div><div><br>- Dave</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
<br>
<a href="https://reviews.llvm.org/D24152" rel="noreferrer" target="_blank">https://reviews.llvm.org/D24152</a><br>
<br>
<br>
<br>
</blockquote></div></div>