Skip to content
This repository has been archived by the owner on Feb 2, 2023. It is now read-only.

Use the standard convention for function pointers in libcruby #134

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

sgrif
Copy link
Collaborator

@sgrif sgrif commented Nov 8, 2017

Typically you never take extern "C" fn directly, as there is no way to
pass NULL there without using transmute. Bindgen and most other tools
generally use Option for this purpose.

I've also changed the alloc and free functions to use the functions
provided on box for converting to/from pointers, rather than relying on
its internal representation. There's still the transmute on the class,
but I'm not entirely sure what that's doing so I've left it alone for
now.

Typically you never take `extern "C" fn` directly, as there is no way to
pass `NULL` there without using transmute. Bindgen and most other tools
generally use `Option` for this purpose.

I've also changed the alloc and free functions to use the functions
provided on box for converting to/from pointers, rather than relying on
its internal representation. There's still the `transmute` on the class,
but I'm not entirely sure what that's doing so I've left it alone for
now.
@wycats
Copy link
Contributor

wycats commented Nov 28, 2017

I'd like to discuss this with some Rust people before making this change.

@sgrif
Copy link
Collaborator Author

sgrif commented Jan 10, 2018

Did you ever get a chance to discuss this with some Rust people?

@wycats
Copy link
Contributor

wycats commented Jan 11, 2018

@sgrif I did... I'm not convinced by your assertion that this is the "standard convention", nor do I know how to feel confident that Option has the representation you expect. @nikomatsakis convinced me that it's "unlikely to change" due to pervasiveness, but I really don't like the idea of depending on happenstances of representation (especially given the fact that fn representation has changed in the recent past).

@sgrif
Copy link
Collaborator Author

sgrif commented Jan 11, 2018

I called it the "standard convention", since it's what bindgen generates,

@wagenet
Copy link
Collaborator

wagenet commented Mar 21, 2018

@wycats @sgrif what's the status here?

@wycats
Copy link
Contributor

wycats commented Jul 29, 2019

Is this superseded by #160?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants