-
Notifications
You must be signed in to change notification settings - Fork 37
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
Provide Try{Into|From}Ctx impls for fixed byte array. #86
Conversation
needs rustfmt |
Also i'm curious how you got this building? Last time I tried this, I got overlapping impl errors, iirc |
Do you mean, the last time you tried implementing this? |
Yea, last time i tried doing what you wrote basically, I recall I got a duplicate impl error due to a blanket impl |
Interesting. |
The CI failed, due to an out of date compiler. |
looks like CI Is failing because rayon bumped their minimum rustc, i'd just update the compiler in CI config since it isn't a real dep |
technically speaking, this will also bump the msrv to whatever the version of rust const generics landed, which is fine, but we should probably but an MSRV in cargo.toml? |
I just looked up in which release this was stabilized and found 1.51. However the |
ah, 1.69 is quite new. So I generally like to keep the MSRV as low as possible (and change it as little as possible), to avoid breaking people (like rayon just did). This is also used in goblin which has an explicit MSRV of 1.60, so that would cause a bump of that (at very least) if we bumped to 1.69 We can punt on the bytes function for now, and merge your other stuff though; also switching to a safe function in std library might be worth the bump along the way to 1.0 |
We could likely resolve this, by including |
Yea, sorry, in general there is an unspoken "no deps in scroll" rule :) There was talk once of adding uuid as an optional dep (to allow the TryFrom impls to be in scroll itself), but that never panned out, and not sure if it was a good idea anyway/kind of niche. |
Ok. Until then I'm just gonna reuse the old iterator approach. If we should ever bump the version to 1.69 we can just switch to the other method. |
On the note of const generics, we could remove the size param from the |
I fixed the CI - MSRV in #90 |
@nyurik Thanks! |
Any further changes needed? |
you may want to rebase on top of main? |
* Set minimum rust version to 1.63 due to rayon-core MSRV requirements, and add a matching `rust-version` in the Cargo.toml
* Add nostd tests * Fix `cargo test --no-default-features` to pass by making std-only tests/docs conditional (added to CI) * There are a few `FIXME` in the code where things seemed suspicious
I just rebased it, but there is one conflict remaining, which I can't fix. |
why exactly can't you fix the conflict? |
i fixed it, it was just the 0x4a vs 04a issue |
looks like failing from a rustfmt now |
* Minor linting * Optimize/cleanup use statements by using `cargo +nightly fmt -- --config imports_granularity=Module,group_imports=StdExternalCrate` * Do not expose `scroll_derive` as a feature in Cargo.toml * removing unneeded allow
Make code a bit easier to read, and also, per rust issue 112156, there is a 6% perf hit when using refs with format, so this PR also fixes that.
@Frostie314159 apologies for the churn on this, would you mind rebasing? I would like to see this get merged |
* Set minimum rust version to 1.63 due to rayon-core MSRV requirements, and add a matching `rust-version` in the Cargo.toml
* Add nostd tests * Fix `cargo test --no-default-features` to pass by making std-only tests/docs conditional (added to CI) * There are a few `FIXME` in the code where things seemed suspicious
It's done. |
@Frostie314159 i have no idea how, but for some reason this PR does not show diff relative to the current main branch, but instead relative to some older revision. As long as @m4b uses "squash and merge", it should be OK though. For the future, I think it is best to not use your main branch to create a PR - makes things a bit confusing and complicated. Instead, create a dedicated branch just for the PR you want to submit. One more thing - it might simplify things for this PR if you squash all your changes, and make them on top of the current main branch - this way everyone would see just the changes you are submitting. Thanks for hard work! |
Yea, I'm very confused what is happening to these PRs, I've never seen github show all the changes like this, it makes it very difficult (and somewhat nerve wracking tbh) to review. I'm somewhat hesitant to merge 25 commits into master; if it doesn't show up as a single squashed commit, I'll likely have to force push the actual content of the PR, which I don't really want to do. |
I just created a new PR so that it is easy to see what is going on with this one - #93 It seems like it may need some work still :( |
P.S. I made some comments on the #93 , which is identical to this PR (and attributes to original author), but still has some issues. My recommendation: create a new branch from the current main branch of the impl<'a, const N: usize> TryFromCtx<'a> for [u8; N] {
type Error = error::Error;
fn try_from_ctx(from: &'a [u8], _ctx: ()) -> Result<(Self, usize), Self::Error> {
// Unwrap is OK, since pread_with already asserts the length.
Ok((from.pread_with::<&'a [u8]>(0, N)?.try_into().unwrap(), N))
}
}
impl<const N: usize> TryIntoCtx for [u8; N] {
type Error = error::Error;
fn try_into_ctx(self, from: &mut [u8], _ctx: ()) -> Result<usize, Self::Error> {
from.pwrite(self.as_slice(), 0)
}
}
impl<'a, const N: usize> TryIntoCtx for &'a [u8; N] {
type Error = error::Error;
fn try_into_ctx(self, from: &mut [u8], ctx: ()) -> Result<usize, Self::Error> {
(*self).try_into_ctx(from, ctx)
}
} |
I think it's best if I close these PRs and start fresh, from the current master and then first implement the clippy fixes, then the unsafe and then the array impls. |
Closed in favor of #95 . |
This PR provides implementations for reading and writing
[u8; N]
.