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

Explain BoundMethod and function_id a bit more #4739

Open
wants to merge 3 commits into
base: trunk
Choose a base branch
from

Conversation

danakj
Copy link
Contributor

@danakj danakj commented Dec 23, 2024

No description provided.

@danakj danakj requested a review from josh11b December 23, 2024 19:59
@github-actions github-actions bot requested a review from jonmeow December 23, 2024 20:00
@danakj danakj enabled auto-merge December 23, 2024 21:56
@danakj danakj requested a review from jonmeow January 6, 2025 19:24
Comment on lines +400 to +402
// 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.
Copy link
Contributor

@jonmeow jonmeow Jan 6, 2025

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

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)

@josh11b josh11b removed their request for review January 6, 2025 22:25
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