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.

Sign up for GitHub

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

Explain BoundMethod and function_id a bit more #4739

Open
wants to merge 3 commits into
base: trunk
Choose a base branch
from
+6 −2
Open

Explain BoundMethod and function_id a bit more #4739

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
658ec99
Explain BoundMethod and function_id a bit more
danakj Dec 23, 2024
6775b20
Update toolchain/sem_ir/typed_insts.h
danakj Jan 6, 2025
d50ed05
Add function_id comment
danakj Jan 6, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions toolchain/sem_ir/typed_insts.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -384,8 +384,9 @@ struct BoolLiteral {
BoolValue value;
};

// A bound method, that combines a function with the value to use for its
// `self` parameter, such as `object.MethodName`.
// For member access such as `object.MethodName`, combines a member function
// with the value to use for `self`. This is a callable structure; `Call` will
// handle the argument assignment.
struct BoundMethod {
static constexpr auto Kind = InstKind::BoundMethod.Define<Parse::NodeId>(
{.ir_name = "bound_method",
Expand All @@ -396,6 +397,9 @@ struct BoundMethod {
// `self`, or whose address will be used to initialize `self` for an `addr
// self` parameter.
InstId object_id;
// The instruction from which to find the function being bound to an object.
// The actual function (e.g. FunctionType) comes from the this instruction's
// type id.
Comment on lines +400 to +402
Copy link
Contributor

@jonmeow jonmeow Jan 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Looking at member_access.cpp, BoundMethod will only be created for FunctionType. I'm not sure I'd call that "the actual function"; to me, that's either the ID here, or the Function object which contains the function implementation. That is, we're not using this to find the bound function, it is the bound function.

What do you think of:

Suggested change
// The instruction from which to find the function being bound to an object.
// The actual function (e.g. FunctionType) comes from the this instruction's
// type id.
// The function being bound, whose type_id is always a `FunctionType`.

Copy link
Contributor

@jonmeow jonmeow Jan 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Sidenote, I think this could maybe be renamed to function_decl_id if you care -- we typically use function_id for FunctionId, but that's not the case here. I do think though that this will match the decl concept (although it may be an ImportRef instead of a FunctionDecl sometimes, I think that matches decl concept for me? I'm not strictly verifying the types either)

InstId function_id;
};

Expand Down
Loading