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

Use new mem::uninitialized API #806

Merged
merged 1 commit into from
Jul 9, 2019

Conversation

GuillaumeGomez
Copy link
Member

@GuillaumeGomez GuillaumeGomez commented Jul 6, 2019

Fixes #770.

cc @sdroege @EPashkin

@GuillaumeGomez
Copy link
Member Author

An example of the result: gtk-rs/pango#155

@sdroege
Copy link
Member

sdroege commented Jul 6, 2019

See comments there, invalid usage of MaybeUninit :)

@GuillaumeGomez
Copy link
Member Author

More like lazy usage but yes. ;) I'll update the PR later on.

@sdroege
Copy link
Member

sdroege commented Jul 6, 2019

Not lazy, it's undefined behaviour just like with mem::uninitialized now. It's wrong :)

@GuillaumeGomez
Copy link
Member Author

I wanted to mean that my implementation was lazy. :)

@sdroege
Copy link
Member

sdroege commented Jul 6, 2019

Oh that's possible :) Changing the code generator to use MaybeUninit correctly is probably quite some work

@GuillaumeGomez GuillaumeGomez force-pushed the new-mem-uninitialized branch 3 times, most recently from c664d93 to 3599ded Compare July 7, 2019 13:33
@GuillaumeGomez
Copy link
Member Author

This is finally done. T_T

.map(|param| {
let kind = type_mem_mode(env, param);
;
(param, kind)
Copy link
Member

Choose a reason for hiding this comment

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

Unneeded debug change?

Copy link
Member Author

Choose a reason for hiding this comment

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

Indeed, good catch.

@GuillaumeGomez GuillaumeGomez force-pushed the new-mem-uninitialized branch 2 times, most recently from 414a3ef to 25028ca Compare July 7, 2019 14:35
@EPashkin
Copy link
Member

EPashkin commented Jul 7, 2019

Thanks, look good for me, it still need cargo fmt run

if name == "error" {
format!(", {} as usize", name)
} else {
format!(", {{ let {0} = {0}.assume_init(); {0} as usize }})", name)
Copy link
Member

Choose a reason for hiding this comment

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

Seems this can be just format!(", {}.assume_init() as usize)", name) without "error" branch

Copy link
Member

Choose a reason for hiding this comment

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

@GuillaumeGomez If you don't want this one-liner, please, merge this PR yourself.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, error branch still needed.
Can you tell me which function?

Copy link
Member Author

Choose a reason for hiding this comment

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

I updated it to a one-liner. However, I don't understand your last comment. What error?

Copy link
Member

Choose a reason for hiding this comment

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

I meant branch if name == "error", I suspect that it never reached:
note that it not ends with ')',
and I updated it to produce bad code, but gtk-rs crates still builds fine after regen.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll remove this check and see the result.

@sdroege
Copy link
Member

sdroege commented Jul 7, 2019

Can you regenerate the pango PR so we can also check the results? :)

@GuillaumeGomez
Copy link
Member Author

@EPashkin You were right: the condition was completely useless so I removed it.

@EPashkin
Copy link
Member

EPashkin commented Jul 9, 2019

@GuillaumeGomez Thanks

@EPashkin EPashkin merged commit fb0b31c into gtk-rs:master Jul 9, 2019
@GuillaumeGomez
Copy link
Member Author

I'll send the PRs soon then!

@GuillaumeGomez GuillaumeGomez deleted the new-mem-uninitialized branch July 9, 2019 19:00
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.

Use MaybeUninit instead of mem::unitialized
3 participants