Page MenuHomePhabricator

[Sema] Defer checking constexpr lambda until after we've finished the lambda class.
ClosedPublic

Authored by erik.pilkington on Apr 2 2018, 6:28 PM.

Details

Summary

Previously, this caused ExprConstant to assert while verifying the lambda is constexpr:

void f() {
    int x = 0;
    [=]() constexpr {
        return x;
    };
}

The problem is that ActOnFinishFunctionBody evaluated the lambda's body before BuildLambdaExpr finished the lambda class. This patch fixes the problem by moving that check to BuildLambdaExpr.

PR36054

Thanks for taking a look!
Erik

Diff Detail

Repository
rL LLVM

Event Timeline

erik.pilkington created this revision.Apr 2 2018, 6:28 PM

Thanks for working on this fairly embarrassing bug (let's fix this before the week is over :)

clang/lib/Sema/SemaDecl.cpp
12886 ↗(On Diff #140722)

Hmm - as opposed to further leaking our lambda's implementation-details into ActOnFinishFunctionBody - and duplicating these checks for a lambda marked constexpr, I think I would prefer teaching the expression-evaluator (ExprConstant.cpp) to know how to check a lambda's function call operator for constexprness (i.e. avoid creating the capture-to-field-map when 'checkingPotentialConstantExpression()' and ignore evaluating expressions that refer to enclosing variables since their evaluation should not affect inferring constexprness).

Thoughts?

erik.pilkington marked an inline comment as done.

Handle this in ExprConstant instead of in Sema.

clang/lib/Sema/SemaDecl.cpp
12886 ↗(On Diff #140722)

That's a really good point, makes this a lot simpler. Thanks!

Thanks Erik!

clang/lib/AST/ExprConstant.cpp
4312 ↗(On Diff #141001)

How about we add a comment here along the lines of: Do not attempt to create the variable-reference to closure data member map while 'constexpr' checking a lambda's function call operator (standard reference). Currently constpexpr checking is done right after the end of the function definition for the syntehsized call operator marked explicitly constexpr - which occurs prior to adding the captures map to the closure object. Alternatively we could have conditioned the check at the end of the function body to bypass lambda call operators and then invoke the constexpr check once the lambda is completely processed.

Between you and me, I'm a little torn about this approach - if you can make an argument to consider your approach over this one - i think i could be swayed (if i'm not already ;) - unless of course richard weighs in as a tie breaker.

5205 ↗(On Diff #141001)

I think you might want to add a check here to determine if E refersToEnclosingVariableOrCapture()?

erik.pilkington marked an inline comment as done.

Add refersToEnclosingVariableOrCaptureCheck(), comments.

clang/lib/AST/ExprConstant.cpp
4312 ↗(On Diff #141001)

Sure, added a version of that comment in the patch. I could go either way here too, but I like your approach because a) it makes the constant evaluator do just the work that is necessary when checkingPotentialConstantExpression(), and b) special-casing here is simpler and more precise than doing it in Sema.

LGTM - can you commit?
Thank you!

clang/lib/AST/ExprConstant.cpp
4313 ↗(On Diff #141042)

should we say something about only explicitly specified constexpr checking (as opposed to constexpr inference) is done prior to capture information being stored in the CXXRecordDecl? *shrug*.

Also the upside of having the expression evaluator evaluate these expressions is we could diagnose such lambdas as ill-formed (though no-diagnostic required per the standard):

void f() {
  volatile int v = 1;
  [&] () constexpr { return v; };  // can never be a core constant expression, right?
}

I think I agree w you though - so, for now lets stick with this version of the patch?

5212 ↗(On Diff #141042)

How about something along the lines of : We don't always have a complete capture-map when checking or inferring if the function call operator meets the requirements of a constexpr function - and we don't need to evaluate the captures to determine constexprness (dcl.constexpr C++17)?

LGTM - can you commit?

Yep, I'll do that now. Thanks for reviewing!

clang/lib/AST/ExprConstant.cpp
5212 ↗(On Diff #141042)

Sure, I just copied that verbatim into the commit.

This revision was not accepted when it landed; it landed in state Needs Review.Apr 4 2018, 5:15 PM
This revision was automatically updated to reflect the committed changes.