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

[Faisal Vali via Phabricator via cfe-commits]

faisalv added inline comments.


================
Comment at: lib/Parse/ParseExprCXX.cpp:1090
+  TemplateParameterDepthRAII CurTemplateDepthTracker(TemplateParameterDepth);
+  Actions.RecordParsingTemplateParameterDepth(TemplateParameterDepth);
+
----------------
hamzasood wrote:
> faisalv wrote:
> > Since you only really need to pass this information on for computing the depth of the 'auto' parameters - why not just leave the RecordParsingTEmplateParameterDepth call where it was, increment the template depth once we parse the TPL, and just pass in the right depth (minus one if ExplicitTemplateParameters) and increment the tracker if we getGenericLambda but no explicit TPL?
> > 
> > I wonder if that might be the safer way to do it - especially if you have generic lambdas in default arguments of generic lambdas - each of which have explicit template parameters also??
> > 
> > thoughts?
> > 
> While the depth is currently only used for auto parameters, that could change in the future (especially since the function name is quite general. I.e. record depth instead of record auto depth or whatever). Because of that, it wouldn't seem right to not record it in the case where there's an explicit template param list but no function parameters. So the options were to record it twice (parsing the explicit TPL and function parameters) or just do it once for every lambda. Since it's a cheap operation (pretty much just setting a variable), I went for the latter.
> 
> Good point about incrementing after seeing an explicit template param list. I'll change that in the next update.
I think If the depth will be needed in the future for something other than determining the depth of the auto parameter - then that change (of moving the call outside) should occur in that 'future' patch.  I don't see why you say that it would ever need to be called twice in your patch?

Just like we localize our variable declarations and limit their scope (to ease comprehension) similarly, i think changes to state should be as local to the part of the program that uses that state (once again for comprehension:  If you move the call, folks now might have to put in more effort when trying to comprehend the effects of that call, since the set of potential operations relying on that set grows).



================
Comment at: lib/Parse/ParseExprCXX.cpp:1305
 
+  TemplateParamScope.Exit();
+
----------------
hamzasood wrote:
> faisalv wrote:
> > Why do you exit the scope here, and then re-add the template parameters to the current scope?  What confusion (if any) occurs if you leave this scope on?
> > 
> That was my original attempt, but I just couldn't get it to work.
> 
> E.g.
> 
> ```
> auto l = []<typename T>(T t) { };
> ```
> 
> triggers an assertion:
> 
> ```
> (!A->getDeducedType().isNull() && "cannot request the size of an undeduced or dependent auto type"), function getTypeInfoImpl, file /Users/hamzasood/LLVM/src/tools/clang/lib/AST/ASTContext.cpp, line 1883.
> ```
> 
> A lot of other things broke too.
> 
> I figured since function parameters are explicitly added to scope, it would make sense to introduce explicit template parameters in the same place.
Hmm - I think at the very least we should understand exactly what assumptions are being violated - and then decide whether we wish to temporarily bypass/shortcut the way scopes are supposed to 'seamlessly' work (w a fixme or comment prior to the 'false exit' that explains why we are doing so) or adjust the assumptions...


https://reviews.llvm.org/D36527