[PATCH] D24152: Support the overloadable attribute with _Generic expressions

[Aaron Ballman via cfe-commits]

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.


https://reviews.llvm.org/D24152