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

Incoming breakage due to static assert on size #56

Closed
oli-obk opened this issue Apr 5, 2022 · 6 comments
Closed

Incoming breakage due to static assert on size #56

oli-obk opened this issue Apr 5, 2022 · 6 comments

Comments

@oli-obk
Copy link

oli-obk commented Apr 5, 2022

rust-lang/rust#94075 optimizes repr(Rust) enums better, making them smaller in many cases. This breaks ropey (see https://crater-reports.s3.amazonaws.com/pr-94075/try%23419e70b02dc61a51433bb55fe5a482c6ab6f7da5/gh/cessen.led/log.txt for an example). I'm not sure when this change gets merged and will hit stable, but relying on the exact size of repr(Rust) types is suboptimal. The easy fix is to use repr(C) for the affected enums, which will disable any such optimizations.

If you have opinions on this topic, please don't hesitate to let me know here or post on the PR.

@cessen
Copy link
Owner

cessen commented Apr 6, 2022

Ah! Thanks for the heads up. I'll look into this soon.

cessen added a commit that referenced this issue Apr 8, 2022
This is so we can rely on their memory layout, which is needed for
achieving a precise `Node` size in memory.

Hopefully addresses issue #56.
@cessen
Copy link
Owner

cessen commented Apr 8, 2022

@oli-obk:
I've pushed what I believe is a fix to master in commit dafa0b6. If you could try building it with rust-lang/rust#94075 and let me know if it's actually fixed, I'd really appreciate it! Then I can cut a new Ropey release with the fix.

@oli-obk
Copy link
Author

oli-obk commented Apr 12, 2022

You can try this out locally by using https://crates.io/crates/rustup-toolchain-install-master to install the toolchain with hash 419e70b02dc61a51433bb55fe5a482c6ab6f7da5

@cessen
Copy link
Owner

cessen commented Apr 12, 2022

Ah, thanks! That's convenient. I assumed I would have to build the whole thing myself, which... sounded quite daunting, especially just to test it out.

@cessen
Copy link
Owner

cessen commented Apr 12, 2022

Looks like it's fixed. Thanks!

I'll leave this issue open until I cut a release with the fix.

@cessen
Copy link
Owner

cessen commented May 29, 2022

Published in Ropey 1.5.0.

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

No branches or pull requests

2 participants