Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor finalization of initializers. #3250

Merged
merged 2 commits into from
Sep 21, 2023

Conversation

zygoloid
Copy link
Contributor

Factor out the common code to find the return slot for an initializing expression, and use it to simplify the two different ways we finalize initializers, as either initializing a temporary or initializing some specific object.

This slightly changes the SemIR we create for function calls: instead of rewriting the Call node to have a different destination and replacing its temporary with a no_op, we now replace its temporary with a stub_reference to the new destination. This results in the same amount of SemIR being produced, but allows calls and other kinds of initializers to be handled uniformly.

Factor out the common code to find the return slot for an initializing
expression, and use it to simplify the two different ways we finalize
initializers, as either initializing a temporary or initializing some
specific object.

This slightly changes the SemIR we create for function calls: instead of
rewriting the Call node to have a different destination and replacing
its temporary with a `no_op`, we now replace its temporary with a
`stub_reference` to the new destination. This results in the same amount
of SemIR being produced, but allows calls and other kinds of
initializers to be handled uniformly.
Copy link
Contributor

@chandlerc chandlerc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LG!

I've left a comment about a recovery case, but its not blocking.

semantics_ir().ReplaceNode(
temporary_id, SemIR::Node::NoOp::Make(temporary.parse_node()));
if (!semantics_ir.GetFunction(callee_id).return_slot_id.is_valid()) {
return SemIR::NodeId::Invalid;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, is this going to cause the caller to restructure how the return is managed (making it not in-place)? For an invalid function, that seems like it might result in cascading errors. Should this instead return a return slot that is marked invalid? Or in some other way model that distinction?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The situation being handled here is that the function doesn't have a return slot (which is modeled by the return slot ID being marked as invalid). Unfortunately, we fairly pervasively use SemIr::NodeId::Invalid to mean "none" (not a valid ID) rather than "the program is invalid". Perhaps we should rename it.

@zygoloid zygoloid added this pull request to the merge queue Sep 21, 2023
Merged via the queue into carbon-language:trunk with commit caecdc7 Sep 21, 2023
6 checks passed
@zygoloid zygoloid deleted the toolchain-get-return-slot branch September 21, 2023 19:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants