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

Deprecate IntKey #472

Closed
maurolacy opened this issue Oct 5, 2021 · 7 comments · Fixed by #547
Closed

Deprecate IntKey #472

maurolacy opened this issue Oct 5, 2021 · 7 comments · Fixed by #547
Assignees
Milestone

Comments

@maurolacy
Copy link
Contributor

maurolacy commented Oct 5, 2021

We can implement PrimaryKey over integer types easily using macros.

Like in

macro_rules! integer_de {
(for $($t:ty),+) => {
$(impl KeyDeserialize for IntKey<$t> {
type Output = $t;
#[inline(always)]
fn from_vec(value: Vec<u8>) -> StdResult<Self::Output> {
Ok(<$t>::from_be_bytes(value.as_slice().try_into()
.map_err(|err: TryFromSliceError| StdError::generic_err(err.to_string()))?))
}
})*
}
}

and then

integer_de!(for i8, u8, i16, u16, i32, u32, i64, u64, i128, u128);

That means, we can remove the need for IntKey, and use the integer values directly. So, better usability, clarity, and user experience in general.

@ethanfrey
Copy link
Member

We cannot currently do this as PrimaryKey has key(&self) -> Vec<&[u8]> which is efficient for bytes and string, but cannot be implemented on u32.

Proposal by @hashedone to use some associated type on the trait, type KeyType: AsRef<[u8]>.

@maurolacy maurolacy self-assigned this Oct 18, 2021
@ethanfrey
Copy link
Member

ethanfrey commented Oct 18, 2021

If you do this in a backwards-compatible way that doesn't break the current usage of IntKey, then I would be happy to get this in 0.10.1. And we just deprecate the IntKey in 0.11.0 (rather than some warnings popping up in a patch release)

@maurolacy
Copy link
Contributor Author

maurolacy commented Oct 18, 2021

Taking a stab at this, and I don't think it's possible. As @ethanfrey said, we need a container / wrapper to store the representation of the type, and be able to return a reference to that.

If I construct the repr on the fly using from_be_bytes(), it's a temporary value, and I cannot reference it.

Ways around this are:

  • Return owned values (i.e. Vec<Vec<u8>>), instead of references.
  • Return boxed values (i.e. Vec<Box<[u8]>>).
  • Implement a wrapper (i.e. IntKey, precisely).

I would love to be proved wrong, but don't think there's another option here. Even with associated types and AsRef, we still need a representation that is not a temporary, to be able to reference it.

The only case that works is for u8 (using std::slice::from_ref()). That is just because the u8 type matches the u8 in the return value (and because, with one byte, you don't run into endianness problems).

If somehow it could be possible to "cast" or "view" an integer as a (big-endian) slice, then we would be able to return a reference to it. But given that there are two internal representations, sometimes (i.e. in little-endian archs, which are the most common) bytes need to be swapped. And that creates a temporary.

Moreover: WebAssembly is (currently) only little-endian (WebAssembly/design#1212).

@ethanfrey
Copy link
Member

Even with associated types and AsRef, we still need a representation that is not a temporary, to be able to reference it.

My understanding was the idea was a trick to allow sometimes to return &[u8] (for all the string / vec types) and sometimes return Vec<u8> for the integer types (u32, i64, ...)

If we have to return &[u8] everywhere, I agree there is no way.
And I feel returning Vec<u8> everywhere is needless runtime overhead to remove a helper type.

@ethanfrey
Copy link
Member

I think @ueco-jb wanted to take a stab at this one, maybe he loves associated types more?

@maurolacy
Copy link
Contributor Author

I think @ueco-jb wanted to take a stab at this one, maybe he loves associated types more?

Sure. I was talking with @hashedone and yes, the alternative would be to have something like a hybrid / variable return type.

@ueco-jb
Copy link
Contributor

ueco-jb commented Nov 15, 2021

Followed up in #549.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants