Page MenuHomePhabricator

Implemented P0428R2 - Familiar template syntax for generic lambdas
ClosedPublic

Authored by hamzasood on Aug 9 2017, 8:47 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
faisalv added inline comments.Aug 18 2017, 8:32 AM
lib/Parse/ParseExprCXX.cpp
1112 ↗(On Diff #110409)

good idea - i think that should work too. (Although i do still like the idea of refactoring this long function via extract-method - but i can always do that refactoring post this patch).

hamzasood updated this revision to Diff 111812.Aug 19 2017, 3:58 AM

Changed a documentation comment to use \brief instead of stating the function name.
Only enter the template parameter scope if needed.
Changed the phrasing of the diagnostic when an empty template parameter list is encountered.

Since submitting this patch for review, a test has been added which fails with this patch. The test has a FIXME comment about moving the test somewhere when template lambda syntax is supported, but I'm not sure what needs to be done. Any ideas?

hamzasood marked 2 inline comments as done.Aug 19 2017, 3:59 AM
faisalv added inline comments.Aug 19 2017, 11:42 AM
include/clang/Sema/ScopeInfo.h
774 ↗(On Diff #111812)

typo: explicitly

lib/Parse/ParseExprCXX.cpp
1090 ↗(On Diff #111812)

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?

1113 ↗(On Diff #111812)

make this const pls.

1305 ↗(On Diff #111812)

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?

In regards to that failing test (that was added since review began) - could you fix that test pls (i.e. rename the nested ttp 'U' to something else) and move it into the function 'f' as requested by the author?
Might want to include a similar (but not same) example of matching complexity to your tests too.

Thanks!

hamzasood added inline comments.Aug 20 2017, 5:11 AM
lib/Parse/ParseExprCXX.cpp
1090 ↗(On Diff #111812)

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.

1305 ↗(On Diff #111812)

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.

faisalv added inline comments.Aug 20 2017, 6:34 AM
lib/Parse/ParseExprCXX.cpp
1090 ↗(On Diff #111812)

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).

1305 ↗(On Diff #111812)

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...

hamzasood updated this revision to Diff 111887.Aug 20 2017, 7:41 AM
  • Corrected a typo.
  • Made HasExplicitTemplateParams const.
  • Reverted the change in where template depth is incremented and recorded.
  • Fixed the broken test.
  • Keep the template scope active instead of exiting it and manually introducing the template parameters into scope. I think this works fine now; you can see the fix in line 847 of SemaLambda.cpp. Basically having a template scope active resulted in the lambda being incorrectly marked as dependent, which broke a lot of stuff.

One question I have is how to expose explicit template params in the AST (if at all)? I was thinking of adding an int representing the number of template params that were explicitly specified. But do I add that to LambdaExpr or CXXRecordDecl::LambdaDefinitionData (or both)?

hamzasood updated this revision to Diff 111892.Aug 20 2017, 9:04 AM

Sorry, I've just spotted a small mistake in how the auto parameter depth is recorded.
This update fixes it.

In regards to representing this in the AST - I think (based on precedence) that the number of explicit template parameters should be stored in LambdaDefinitionData - and the interface exposed through LambdaExpr (where the source information of the template parameter list should be stored too i think - Richard you agree?).

Also can you add examples of such generic lambdas that are nested within either other generic lambdas or templates - and make sure that they instantiate/substitute correctly - and that we really don't have to touch the template instantiation machinery?

Thanks!

include/clang/Sema/ScopeInfo.h
955 ↗(On Diff #111892)

This function doesn't seem to be called anywhere?

lib/Parse/ParseExprCXX.cpp
1116 ↗(On Diff #111892)

Why not Just use/pass LSI->TemplateParams?

lib/Sema/SemaLambda.cpp
501 ↗(On Diff #111892)

I'm not sure i see the point in creating the template parameter list here and returning it, when it's not used anywhere? The TPL is created and cached in GLTemplateParameterList when needed it seems.

858 ↗(On Diff #111892)

Hmm - now that you drew my attention to this ;) - I'm pretty sure this is broken - but (embarrassingly) it broke back when i implemented generic lambdas (and interestingly is less broken w generic lambdas w explicit TPLs) - could I ask you to add a FIXME here that states something along the lines of:

When parsing default arguments that contain lambdas, it seems necessary to know whether the containing parameter-declaration clause is that of a template to mark the closure type created in the default-argument as dependent. Using template params to detect dependence is not enough for all generic lambdas since you can have generic lambdas without explicit template parameters, and whose default arguments contain lambdas that should be dependent - and you can not rely on the existence of a template parameter scope to detect those cases. Consider:

auto L = [](int I = [] { return 5; }(), auto a) { };

The above nested closure type (of the default argument) occurs within a dependent context and is therefore dependent - but we won't know that until we parse the second parameter.

p.s. I'll try and get around to fixing this if no one else does.

hamzasood marked 5 inline comments as done.Aug 21 2017, 4:54 AM

So to clarify: NumExplicitTemplateParams should be stored in LambdaDefinitionData (with an accessor in LambdaExpr) and ExplicitTemplateParamRange (SourceRange) should be stored in LambdaExpr?
That makes sense, but doesn't seem consistent with how things are currently stored. E.g. NumCaptures is stored in both LambdaDefinitionData and LambdaExpr.

lib/Parse/ParseExprCXX.cpp
1116 ↗(On Diff #111892)

I thought that Parser and Sema stay separate, and communicate through various ActOn functions? Directly accessing LSI would violate that.

faisalv added inline comments.Aug 21 2017, 5:32 AM
lib/Parse/ParseExprCXX.cpp
1116 ↗(On Diff #111892)

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).

hamzasood added inline comments.Aug 21 2017, 6:31 AM
lib/Parse/ParseExprCXX.cpp
1116 ↗(On Diff #111892)

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.

lib/Sema/SemaLambda.cpp
858 ↗(On Diff #111892)

Good point. Now you mention it, isn't it even more broken than than?
E.g:

auto L = [](auto A, int I = [] { return 5; }());

L is known to be generic before we parse the nested lambda, but template param scopes aren't established for auto parameters (I think), so the nested lambda won't get marked as dependent

faisalv added inline comments.Aug 21 2017, 8:31 AM
lib/Sema/SemaLambda.cpp
858 ↗(On Diff #111892)

I was aware of that - but I think that's the easier case to fix (where you see the auto first - hence i reversed it in my example).

faisalv added inline comments.Aug 21 2017, 8:34 AM
lib/Parse/ParseExprCXX.cpp
1116 ↗(On Diff #111892)

OK - add a FIXME that alerts folks that we currently make two copies of this and ideally we shouldn't need to.

  • Info about a lambda's explicit template parameters is now exposed in the AST (see changes to LambdaExpr). It turns out that no extra storage was actually needed to achieve this.
  • Removed remnants of a previous implementation (unused getExplicitTemplateParams function, creation of a TemplateParameterList in ActOnLambdaTemplateParameterList).
  • Renamed ActOnLambdaTemplateParameterList -> ActOnLambdaExplicitTemplateParameterList (which is a bit more clear).
  • Changed LambdaScopeInfo::TemplateParams from a vector of Decl* to a vector of NamedDecl*.
faisalv added inline comments.Aug 25 2017, 4:11 AM
lib/AST/ExprCXX.cpp
979 ↗(On Diff #112018)

I think this should return an invalid range if getExplicitCount is 0.
might assert that when not 0, langleloc does not equal rangleloc.

lib/Sema/SemaLambda.cpp
486 ↗(On Diff #112018)

Perhaps also assert TParams.size should not be 0?

495 ↗(On Diff #112018)

ack - avoid reinterpret cast please - why not just stick to Decl* for TemplateParams for now - and add some fixme's that suggest we should consider refactoring ParseTemplateParameterList to accept a vector of nameddecls - and update this when that gets updated ?

Perhaps add an assert here that iterates through and checks to make sure each item in this list is some form of a template parameter decl - within an #ifndef NDEBUG block (or your conversion check to NameDecl should suffice?)

hamzasood added inline comments.Aug 25 2017, 4:37 AM
lib/AST/ExprCXX.cpp
979 ↗(On Diff #112018)

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.

lib/Sema/SemaLambda.cpp
495 ↗(On Diff #112018)

Unfortunately TemplateParameterList::Create expects an array of NamedDecl, so there’ll need to be a forced downcast somewhere. By getting that over with as soon as possible, any errors will be caught straight away by that assertion instead of moving the problem down the line somewhere and making it more difficult to see where it went wrong.

OK - I'll commit this on sunday if no one blocks it by then. (I'll add the fixme's).

Nice Work!

faisalv added inline comments.Aug 25 2017, 6:21 PM
lib/AST/ExprCXX.cpp
979 ↗(On Diff #112018)

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?

Thanks for all of your feedback! It’s really helped to improve this patch.

lib/AST/ExprCXX.cpp
979 ↗(On Diff #112018)

That’s changed in this patch. LAngleLoc and RAngleLoc are invalid unless there’s an explicit template parameter list.

lib/Parse/ParseExprCXX.cpp
1116 ↗(On Diff #111892)

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.

rsmith added inline comments.Aug 27 2017, 5:59 PM
lib/Parse/ParseExprCXX.cpp
638 ↗(On Diff #112018)

We generally put terminals in single quotes in these grammar excerpts:

lambda-introducer '<' template-parameter-list '>' lambda-declarator[opt]
1126–1127 ↗(On Diff #112018)

No linebreak between } and else.

lib/Sema/SemaLambda.cpp
495 ↗(On Diff #112018)

OK, but please cast them one by one rather than type-punning the array (which results in UB).

845–846 ↗(On Diff #112018)

No linebreak between } and else.

test/CXX/temp/temp.decls/temp.variadic/p4.cpp
216 ↗(On Diff #112018)

Please use __cplusplus > 201703L rather than a number that will become meaningless once we have a real value for C++2a.

hamzasood updated this revision to Diff 112867.Aug 28 2017, 3:03 AM

Hi Richard, thanks for your feedback. This update makes the changes that you requested.
It turns out that the casting is no longer an issue since ParseTemplateParameters has been refactored to use NamedDecl.

rsmith edited edge metadata.Aug 28 2017, 1:56 PM

This patch appears to be missing some necessary changes in the following places:

  • lib/Serialization -- round-trip the new information through PCH / modules
  • lib/AST/StmtPrinter.cpp -- pretty-printing lambdas with explicit template parameters
  • lib/Sema/TreeTransform.h -- template instantiation of lambdas with explicit template arguments
  • lib/AST/RecursiveASTVisitor.h -- recursive AST visitation should visit explicit template parameters

We'll also need to agree within the cxx-abi-dev list how to mangle lambda closure types for this form and lib/AST/ItaniumMangle.cpp's mangleLambda will need to be updated to match. (The MS mangling is not based on the type of the lambda call operator, so we probably don't need changes there.)

We'll also need to agree within the cxx-abi-dev list how to mangle lambda closure types for this form

I filed https://github.com/itanium-cxx-abi/cxx-abi/issues/31 for this.

This patch appears to be missing some necessary changes in the following places:

  • lib/Serialization -- round-trip the new information through PCH / modules

Shouldn't the explicit template parameter information be automatically serialized within the non-implicitness of the corresponding template parameter decls within our TemplateParameterList?

  • lib/AST/StmtPrinter.cpp -- pretty-printing lambdas with explicit template parameters

yes - this would be nice.

  • lib/Sema/TreeTransform.h -- template instantiation of lambdas with explicit template arguments

yes - i had wondered about TreeTransform - I had hoped that the transformation of the template parameter list (for all generic lambdas) would be sufficient (perhaps only need to be tweaked to maintain the implicit flag on template parameters, if that's not transferred through?) - but I agree this does need to be tested well (substitution into default template arguments, non-type-template parameters for e.g.).

  • lib/AST/RecursiveASTVisitor.h -- recursive AST visitation should visit explicit template parameters

Would we also want to (explicitly) visit the implicit template parameters?
Interestingly we don't (explicitly) visit the lambda's function parameters?

We'll also need to agree within the cxx-abi-dev list how to mangle lambda closure types for this form and lib/AST/ItaniumMangle.cpp's mangleLambda will need to be updated to match. (The MS mangling is not based on the type of the lambda call operator, so we probably don't need changes there.)

Interestingly we don't (explicitly) visit the lambda's function parameters?

This is obviously false - sorry :)

hamzasood updated this revision to Diff 113271.Aug 30 2017, 8:54 AM

Implemented pretty printing and recursive AST visiting (both with appropriate unit tests).

The pretty printing implementation includes fixing a few printing bugs (which I needed fixed to write decent tests);

  • Unnamed template parameters now print without a trailing space (e.g. <class> instead of <class >).
  • C++14 generic lambda parameters are now printed as auto as opposed to typeparameter-depth-index.

RecursiveASTVisitor now visits each explicit template parameter, and also the implicit ones if shouldVisitImplicitCode is set.
However there is a problem with this implementation that I'm not sure about.
Consider a TU that is simply: auto l = []<typename T>() { };
If shouldVisitImplicitCode is set, the template parameter will be visited twice, Once when visiting the lambda (which is what I added) and once when visiting the implicit lambda class type (existing behaviour). Is that a problem?

With regards to serialisation, I don't think any changes are needed. However I've added a PCH test to be sure (which passes).

Is that a problem?

I don't think so, because shouldVisitImplicitCode() doesn't mean that it only visits implicit code, it's not onlyVisitImplicitCode() :) In fact, I would be surprised if the template parameters were only visited once.

lib/AST/ExprCXX.cpp
21 ↗(On Diff #113271)

You don't need this header.

1010 ↗(On Diff #113271)

You could store the lambda in a variable instead of having two times the same exact lambda expression.

lib/Sema/SemaLambda.cpp
840 ↗(On Diff #113271)

I think you should add an assert to verify that getParent() doesn't return nullptr, just to be safe from UB.

unittests/AST/StmtPrinterTest.cpp
126 ↗(On Diff #113271)

LLVM style guide says that if you are using a braced-init-list to initialize an object, you have to use an =.

hamzasood updated this revision to Diff 114550.Sep 11 2017, 2:43 AM
  • Replaced an unneeded STLExtras.h include with <algorithm>.
  • Assert that the lambda template scope has a parent before accessing it.
  • Added an equals in a braced init as per the LLVM coding standards.

Thanks @Rakete1111

A couple of remaining pieces that I think are missing:

  • Tests for instantiation of templates containing one of these lambdas. I would expect you'll find you need to change Sema/TreeTransform.h to make that work.
  • Updates to Itanium mangling (AST/ItaniumMangle.cpp) for the mangling changes described in https://github.com/itanium-cxx-abi/cxx-abi/issues/31
lib/Parse/ParseExprCXX.cpp
1117 ↗(On Diff #114550)

Please add a pre-C++2a-compat warning along this code path, and permit this in pre-C++2a modes with an extension warning. (You can look at other C++2a features for examples of how to do this.)

1151–1165 ↗(On Diff #114550)

This handling of the template depth is more subtle / error-prone than I'd like. Maybe you could add a TemplateParameterDepthRAII::setAddedDepth(1) to replace the conditional increment, and a TemplateParameterDepthRAII::getOriginalDepth() to replace the conditional subtraction of 1?

lib/Sema/SemaLambda.cpp
484–490 ↗(On Diff #114550)

Please put the && on the previous line.

hamzasood updated this revision to Diff 118261.Oct 9 2017, 1:14 PM
  • Updated lambda mangling to include explicit template parameters
  • Allow explicit template parameter lists on lambdas pre-c++2a as an extension.
  • Improved the somewhat fragile template depth handling.
  • Reformatted some asserts.

Could you expand on your first point a bit more? Do you have an example that shows the issue?

hamzasood updated this revision to Diff 123015.Nov 15 2017, 5:09 AM

Resolved merge conflicts from the last month of changes (unittests/AST/StmtPrinterTest.cpp and lib/AST/DeclCXX.cpp)

hamzasood updated this revision to Diff 156693.Jul 21 2018, 3:20 PM
  • Resolved merge conflicts

This looks great, thanks!

Could you expand on your first point a bit more? Do you have an example that shows the issue?

You have very little test coverage for the case where a lambda with explicit template parameters is used within another template. Eg, something like (untested):

template<typename T> constexpr T f() {
  return []<T V>() { return V; }.operator()<12345>(); // expected-error {{no viable function}} expected-note {{candidate}}
}
static_assert(f<int>() == 12345); // ok
int *p = f<int*>(); // expected-note {{in instantiation of}}
lib/AST/DeclCXX.cpp
1396–1397 ↗(On Diff #156693)

Given that you've already assert-checked is_partitioned, maybe use a binary search here (eg, lower_bound)?

lib/AST/DeclPrinter.cpp
107–108 ↗(On Diff #156693)

Nit: I'd prefer splitting this into two functions (one that prints 'template', calls the other, then prints a space, and the other to print the actual template parameters) rather than passing a boolean flag.

lib/AST/ItaniumMangle.cpp
1710 ↗(On Diff #156693)

No newline between } and else, please.

1752–1757 ↗(On Diff #156693)

Might be nice to add an accessor that returns the range of explicit parameters. (The if surrounding this code would then also be redundant.)

lib/Parse/ParseExprCXX.cpp
1139 ↗(On Diff #156693)

You need to do something here to leave the lambda scope that was pushed near the start of this function.

unittests/AST/StmtPrinterTest.cpp
109 ↗(On Diff #156693)

Please commit the refactoring portion of the changes to this file separately.

Submitter: Any update on the status? It seems that Richard's feedback should be trivial enough, so I was wondering if you need this taken up?

hamzasood updated this revision to Diff 177221.Dec 7 2018, 8:00 AM
  • Committed the test framework changes separately as r348589 and r348603.
  • Correctly cleanup the lambda scope if an error in encountered while parsing the template parameters.
  • Replaced getExplicitTemplateParameterCount with getExplicitTemplateParameters, which returns an ArrayRef. This simplifies the handling of the parameters in a few places.
  • Replaced a linear search over the template parameters with a binary search.
  • Added expected error cases to the template lambda usage tests to ensure that the static assertions are correctly instantiated.
  • Added a test for templated lambdas that're used within another template; TreeTransform already transforms the template parameter list so no change was needed to make the test pass.
hamzasood marked 6 inline comments as done.Dec 7 2018, 8:04 AM
hamzasood added inline comments.
lib/AST/DeclPrinter.cpp
107–108 ↗(On Diff #156693)

Do you reckon that the boolean flag is okay for TemplateParameterList::print? I've left this change for now until that has been decided.

rsmith accepted this revision.Sat, May 4, 12:46 AM
rsmith marked an inline comment as done.

Thank you, please go ahead and land this! (We'll need to parse and handle requires-clauses here too, but that can wait for another patch.)

(Please feel free to send ~weekly pings if you're not getting code review feedback; it's easy for things to get missed.)

lib/AST/DeclPrinter.cpp
107–108 ↗(On Diff #156693)

This seems fine.

This revision is now accepted and ready to land.Sat, May 4, 12:46 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptSat, May 4, 3:47 AM