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

[David Blaikie via cfe-commits]

On Fri, Sep 2, 2016 at 10:57 AM Aaron Ballman <aaron.ballman at 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
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20160902/660c6da2/attachment.html>