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

Merged

KodrAus merged 4 commits from feat/128bit-refs into master

2022-04-20 05:47:38 -05:00

KodrAus commented

2022-04-20 01:31:12 -05:00

(Migrated from github.com)

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 https://github.com/rust-lang/rust/pull/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

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 https://github.com/rust-lang/rust/pull/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 commented

2022-04-20 01:33:30 -05:00

(Migrated from github.com)

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.

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.

Thomasdezeeuw (Migrated from github.com) approved these changes

2022-04-20 03:09:31 -05:00

Thomasdezeeuw (Migrated from github.com) left a comment

LGTM.

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

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

src/kv/value.rs

Nit: final ,.

Nit: final `,`.

KodrAus (Migrated from github.com) reviewed

2022-04-20 05:33:06 -05:00

src/kv/value.rs

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

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

KodrAus commented

2022-04-20 05:37:08 -05:00

(Migrated from github.com)

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.

> 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](https://github.com/rust-lang/log/blob/master/Cargo.toml#L48), 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.

👍 1