hamzasood added a comment.
Thanks for all of your feedback! It’s really helped to improve this patch.
================
Comment at: lib/AST/ExprCXX.cpp:979
+SourceRange LambdaExpr::getExplicitTemplateParameterListRange() const {
+ TemplateParameterList *List = getTemplateParameterList();
----------------
faisalv wrote:
> hamzasood wrote:
> > faisalv wrote:
> > > I think this should return an invalid range if getExplicitCount is 0.
> > > might assert that when not 0, langleloc does not equal rangleloc.
> > >
> > I think it does return an invalid range in that case. Without explicit
> > template parameters, getLAngleLoc and getRAngleLoc return invalid
> > locations. And a range of invalid locations is an invalid range. But I
> > suppose that could be asserted to make sure.
> fyi - Since getGenericLambdaTPL sets the LAngleLoc and RAngleLoc to the
> introducer range - unless one handles that case spearately- those funcs won't
> return an invalid loc?
That’s changed in this patch. LAngleLoc and RAngleLoc are invalid unless
there’s an explicit template parameter list.
================
Comment at: lib/Parse/ParseExprCXX.cpp:1116
+ if (HasExplicitTemplateParams) {
+ SmallVector<Decl*, 4> TemplateParams;
+ SourceLocation LAngleLoc, RAngleLoc;
----------------
faisalv wrote:
> hamzasood wrote:
> > faisalv wrote:
> > > hamzasood wrote:
> > > > faisalv wrote:
> > > > > Why not Just use/pass LSI->TemplateParams?
> > > > I thought that Parser and Sema stay separate, and communicate through
> > > > various ActOn functions? Directly accessing LSI would violate that.
> > > Aah yes - you're right. Still it does seem a little wasteful to create
> > > two of those (and then append). What are your thoughts about passing the
> > > argument by (moved from) value, and then swapping their guts within ActOn
> > > (i..e value-semantics) ? (I suppose the only concern would be the small
> > > array case - but i think that would be quite easy for any optimizer to
> > > inline).
> > >
> > I don't think a SmallVectorImpl can be passed by value.
> >
> > So to make that work, the function would either needed to be templated
> > (SmallVector<Decl*, I>) or only accept a SmallVector<Decl*, 4>. And I don't
> > think either of those options are worthwhile.
> OK - add a FIXME that alerts folks that we currently make two copies of this
> and ideally we shouldn't need to.
I disagree with this. I don’t think that needing to copy a few pointers is a
big deal; it happens all the time. E.g. a few lines below this is a vector of
ParamInfo that’s filled locally and then copied to its destination. The
performance gain from eliminating that copy isn’t worth making the code more
confusing, especially since the small case will be hit most of the time.
https://reviews.llvm.org/D36527
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
