Page MenuHomePhabricator

Allow immediate invocation of constructors
AbandonedPublic

Authored by wchilders on Mar 18 2020, 4:37 PM.

Details

Reviewers
rsmith
Tyker
Summary

Hi all,

This is an initial patch submission falling out of Lock3's reflection and metaprogramming work. This is a rather small patch to allow immediate invocation of constructors.

We have additional larger patches for consteval which will likely be a bit trickier, so I wanted to start here to help familiarize myself with the process.

For context, we have forthcoming patches that:

  • Provide improvements which allow the cached values of ConstantExpr to be used by both the constant evaluator and code gen
  • Update the application of evaluation contexts, using the stronger guarantees of manifest constant evaluation to apply the ConstantEvaluated evaluation context in more places

These are both larger, and while we depend on them in observable ways for our work, these are not currently observable changes from the perspective of the test suite.

Diff Detail

Event Timeline

wchilders created this revision.Mar 18 2020, 4:37 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 18 2020, 4:37 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
wchilders retitled this revision from Allow immediate invocations of constructors to Allow immediate invocation of constructors.Mar 18 2020, 4:39 PM
rsmith added inline comments.Mar 18 2020, 6:21 PM
clang/lib/Sema/SemaInit.cpp
6466

It looks like the other callers to BuildCXXConstructExpr are also missing this handling. Can we put the call to CheckForImmediateInvocation in BuildCXXConstructExpr to handle all those cases at once, or do we need to defer it until after the other stuff below?

wchilders marked an inline comment as done.Mar 18 2020, 7:21 PM
wchilders added inline comments.
clang/lib/Sema/SemaInit.cpp
6466

Those changes shouldn't have impact here.

I choose to apply this here as when looking at other uses of CheckForImmediateInvocation, MaybeBindToTemporary was performed on the expr, prior to the expr being passed to CheckForImmediateInvocation.

Upon changing this over, all tests (upstream and ours) still pass, so if you don't see any issues, I'm fine with moving this into BuildCXXConstructExpr. Additionally, upon closer inspection, there is low coupling between MaybeBindToTemporary and CXXConstructExpr, where as there is significant coupling for ObjC and CallExprs. Unless something I've said here gives you some doubts, I'm fine with moving this to BuildCXXConstructExpr, there's nothing that immediately jumps out to me as a problem.

wchilders marked an inline comment as done.Mar 18 2020, 7:27 PM
wchilders added inline comments.
clang/lib/Sema/SemaInit.cpp
6466

Upon changing this over, all tests (upstream and ours) still pass, so if you don't see any issues, I'm fine with moving this into BuildCXXConstructExpr. (Whoops, rearranged some sentences last second heh)

Tyker added a comment.EditedMar 19 2020, 2:25 AM

I have already a patch aiming to do the same thing. D74007

Provide improvements which allow the cached values of ConstantExpr to be used by both the constant evaluator and code gen

this is definitely something we should do.

Update the application of evaluation contexts, using the stronger guarantees of manifest constant evaluation to apply the ConstantEvaluated evaluation context in more places

the tracking of evaluation context isn't as good as it could/should.

wchilders abandoned this revision.Mar 19 2020, 10:04 AM

I have already a patch aiming to do the same thing. D74007

Oof, okay, sounds good. I was not aware of this patch, it looks to be much further along :)

Provide improvements which allow the cached values of ConstantExpr to be used by both the constant evaluator and code gen

this is definitely something we should do.

Update the application of evaluation contexts, using the stronger guarantees of manifest constant evaluation to apply the ConstantEvaluated evaluation context in more places

the tracking of evaluation context isn't as good as it could/should.

With this being closed out in favor of Tyker's patch, I'll post a patch for reusing cached values of ConstantExpr shortly.

I have already a patch aiming to do the same thing. D74007

Thanks. Sorry I dropped the ball on that one. =/