Skip to content

Conversation

@mhammond
Copy link
Member

…t the impl is

@bendk I think this is what we discussed - as you'll see in the patch, I think that either using ObjectImpl is wrong, or what this patch needs more work so that it always gets the precise variant in the places where today it doesn't know what it is.

Seems to work with js, although I've not actually tested it 😅

@mhammond mhammond requested a review from bendk December 23, 2025 03:40
Copy link
Contributor

@bendk bendk left a comment

Choose a reason for hiding this comment

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

These changes look good to me, although I feel like some of my changes in the last commit might have eliminated the special cases that we needed these for in the first place.

In any case, more information doesn't seem bad, maybe some other bindings will want it. For that reason I like using ObjectImpl rather than only distinguishing interface vs trait.

@mhammond
Copy link
Member Author

For that reason I like using ObjectImpl rather than only distinguishing interface vs trait.

But it seems like at least a couple of places in callback_interfaces.rs doesn't actually know whether it is a ::Trait or a CallbackTrait - so languages which care about the distinction might get it wrong?

(At least It think both these variants use vtables in the same way?)

@bendk
Copy link
Contributor

bendk commented Dec 29, 2025

But it seems like at least a couple of places in callback_interfaces.rs doesn't actually know whether it is a ::Trait or a CallbackTrait - so languages which care about the distinction might get it wrong?

Ahh, i didn't realize that. In that case, I'm for a boolean.

@mhammond
Copy link
Member Author

Ahh, i didn't realize that. In that case, I'm for a boolean.

I ended up just adding a new variant - wdyt?

@mhammond mhammond marked this pull request as ready for review December 30, 2025 01:22
@mhammond mhammond requested a review from a team as a code owner December 30, 2025 01:22
@mhammond mhammond requested review from bendk and gruberb and removed request for a team December 30, 2025 01:22
Copy link
Contributor

@bendk bendk left a comment

Choose a reason for hiding this comment

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

Looks good to me. I like the new variant.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants