On Fri, Sep 2, 2016 at 10:57 AM Aaron Ballman <aaron.ball...@gmail.com> wrote:
> aaron.ballman added inline comments. > > ================ > Comment at: lib/Sema/SemaOverload.cpp:12996 > @@ +12995,3 @@ > + // selection expression. > + std::vector<Expr *> AssocExprs(GSE->getAssocExprs().vec()); > + unsigned ResultIdx = GSE->getResultIndex(); > ---------------- > george.burgess.iv wrote: > > aaron.ballman wrote: > > > dblaikie wrote: > > > > george.burgess.iv wrote: > > > > > Is there a reason this isn't a `SmallVector` instead? > > > > Another note on this - we should generally prefer copy init over > direct init (less power, less responsibility/easier to read): > > > > > > > > std::vector<Expr*> AssocExprs = GSE->getAssocExprs().vec(); > > > > > > > > (& 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) > > > 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. > > > 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 > > > > I was thinking that we would end up using the `SmallVector(begin(), > end())` ctor instead, so the vector temp wouldn't be needed. :) > > > > Regardless, it was just a nit, so I'm perfectly happy if it stays a > `vector`. > > > > > I doubt this will wind up on the hot path with any regularity > > > > Agreed. > > I was thinking that we would end up using the SmallVector(begin(), > end()) ctor instead, so the vector temp wouldn't be needed. :) > > 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. > Yeah, we really should (if possible) change this ctor: http://llvm.org/docs/doxygen/html/classllvm_1_1SmallVector.html#a12de5fa9d857aa18e9edac9db6a79838 - to take an opaque range, any range, not just an iterator_range. Then you could construct a SmallVector from an ArrayRef. (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, to make sure the ctor wasn't firing on any/all arguments) - Dave > > > https://reviews.llvm.org/D24152 > > > >
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
