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

Upgrade to value-bag 1.0.0-alpha.9 and remove by-value 128bit int conversions #504

Merged
merged 4 commits into from Apr 20, 2022

Conversation

KodrAus
Copy link
Contributor

@KodrAus KodrAus commented Apr 20, 2022

This PR upgrades our value-bag version to 1.0.0-alpha.9, which includes a breaking change to how 128bit numbers are handled. Instead of storing them by-value we only store them by reference. Along with some other internal layout changes this reduces the size of Value from 48 to 24 bytes, which is a great saving for the stack-local arrays we create to hold them.

In practice this doesn't affect whether or not 128bit values can be used in the macros because they rely on the ToValue trait, which is always by-reference. I'd be keen to hear if anybody is relying on the From impls though.

I'm hoping to get this through sooner rather than later in case rust-lang/rust#95845 is merged in its current state that breaks constant type id comparison, which will mean shipping a patch to value-bag for nightly users.

r? @Thomasdezeeuw

@KodrAus
Copy link
Contributor Author

KodrAus commented Apr 20, 2022

We can work around the breakage if it's worth it by the way. I just carried it through to log because it seemed appropriate.

Copy link
Collaborator

@Thomasdezeeuw Thomasdezeeuw left a comment

Choose a reason for hiding this comment

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

LGTM.

I also took a quick the changes to sval and I think I found a breaking one in sval-rs/value-bag@3fb84ab#diff-2e9d962a08321605940b5a657135052fbcef87b5e360662bb527c96d9a615542R24-R25. It uses ? in enabling feature, which is 1.60+, while log still supports 1.31 (although we could bump that).

impl_to_value_primitive![
usize, u8, u16, u32, u64, u128, isize, i8, i16, i32, i64, i128, f32, f64, char, bool,
];
impl_to_value_primitive![usize, u8, u16, u32, u64, isize, i8, i16, i32, i64, f32, f64, char, bool,];
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: final ,.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that macro implementation is a little iffy and it actually requires that trailing , 😄

@KodrAus
Copy link
Contributor Author

KodrAus commented Apr 20, 2022

It uses ? in enabling feature, which is 1.60+, while log still supports 1.31 (although we could bump that).

Ah well spotted! We actually only require the latest stable for the key-value API, so are ok to bump this. That feature is perfect for that library because it stitches together serialization APIs. Before I was wondering how I could ever introduce a new framework like valuable without exploding the crate graph, but now that's actually possible.

@KodrAus KodrAus merged commit 6c3cd4a into rust-lang:master Apr 20, 2022
@KodrAus KodrAus deleted the feat/128bit-refs branch April 20, 2022 10:47
EFanZh pushed a commit to EFanZh/log that referenced this pull request Jul 23, 2023
Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants