-
-
Notifications
You must be signed in to change notification settings - Fork 106
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
Revert "Remove all transmute calls in generated code" #902
Conversation
This reverts commit 5ca26d9.
src/writer/to_code.rs
Outdated
@@ -137,7 +137,7 @@ impl ToCode for Chunk { | |||
); | |||
let self_str = if in_trait { "Self, " } else { "" }; | |||
let s2 = format!( | |||
"\tSome(*(&{}::<{}F> as *const _ as *const _)), Box_::into_raw(f))", | |||
"\tSome(transmute({}::<{}F> as usize)), Box_::into_raw(f))", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For completeness, can we do
"\tSome(transmute({}::<{}F> as usize)), Box_::into_raw(f))", | |
"\tSome(transmute::<_, unsafe extern "C" fn()>({}::<{}F> as *const ())), Box_::into_raw(f))", |
That's less likely to fall apart and closer to the unsafe code guidelines.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, but without second ::
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, updated comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some(transmute::<_, unsafe extern "C" fn()>(
activate_trampoline::<Self, F> as *const (),
)),
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks correct
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only problem of this change that we need change manual code again instead of just reverting commit :(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to do that anyway, there were some other changes in the manual code that should be kept
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@EPashkin Can you update?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, forgot to push.
Done
Thanks, let's get this in then and then fix up everything else :) |
Part of #901
This reverts commit 5ca26d9.
Comment from #903:
Unfortunately, it wasn't a successful experiment... A solution is still possible without
transmute
:But
transmute
seems to be the simplest solution. So let's go back to it.