[PATCH] D36527: Implemented P0428R2 - Familiar template syntax for generic lambdas

Hamza Sood via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Sat Aug 26 03:02:54 PDT 2017


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





More information about the cfe-commits mailing list