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

Model the return slot as an output parameter #4432

Merged
merged 7 commits into from
Oct 23, 2024

Conversation

geoffromer
Copy link
Contributor

Also fix Param insts to have meaningful names in pretty-printing, to help clarify relationship with return slot.

toolchain/sem_ir/typed_insts.h Outdated Show resolved Hide resolved
Comment on lines 895 to 896
TypeId type_id;
InstId value_id;
Copy link
Contributor

Choose a reason for hiding this comment

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

Comment? I'm guessing type_id matches the return type of the function (though it might be good to separately associate the specific instruction representing that expression in the source for diagnostics). What is the value_id?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, how's this?

(though it might be good to separately associate the specific instruction representing that expression in the source for diagnostics)

Do you see any places we can use that right now? If not, I'd rather wait until we have a need for it.

toolchain/sem_ir/inst_namer.cpp Outdated Show resolved Hide resolved
toolchain/check/call.cpp Outdated Show resolved Hide resolved
toolchain/check/name_component.h Outdated Show resolved Hide resolved
toolchain/sem_ir/typed_insts.h Outdated Show resolved Hide resolved
toolchain/check/handle_function.cpp Show resolved Hide resolved
Copy link
Contributor

@zygoloid zygoloid left a comment

Choose a reason for hiding this comment

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

LGTM, leaving for @josh11b to approve once he's happy.

@@ -42,7 +42,7 @@ static auto NoteReturnType(Context::DiagnosticBuilder& diag,
SemIR::TypeId return_type_id) {
CARBON_DIAGNOSTIC(ReturnTypeHereNote, Note, "return type of function is {0}",
SemIR::TypeId);
diag.Note(function.return_storage_id, ReturnTypeHereNote, return_type_id);
diag.Note(function.return_slot_id, ReturnTypeHereNote, return_type_id);
Copy link
Contributor

Choose a reason for hiding this comment

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

Here is an example where it would be better to use an instruction id instead of a TypeId, so that we could match the spelling of the type the user wrote. We have been using TypeIds previously, but the goal is to switch.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, given an InstId representing the type expression the user wrote, how do I obtain the spelling the user wrote? Is there an existing example of the new pattern that I can follow to update this code?

Copy link
Contributor

Choose a reason for hiding this comment

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

Use the type InstIdAsType as the type of the diagnostic argument; search for that type name for examples.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, done.

@geoffromer geoffromer requested a review from josh11b October 23, 2024 00:27
@josh11b josh11b added this pull request to the merge queue Oct 23, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 23, 2024
@geoffromer geoffromer enabled auto-merge October 23, 2024 16:37
@geoffromer geoffromer added this pull request to the merge queue Oct 23, 2024
Merged via the queue into carbon-language:trunk with commit 9266f86 Oct 23, 2024
8 checks passed
@geoffromer geoffromer deleted the return branch October 23, 2024 17:06
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.

3 participants