Make count_with more consistent for builtin types. #318
Conversation
|
same minor UI test failure on nightly as before. I don't see a reason why this would be more likely to fail on 1.66 than the other, so I'm not bothering to re-run the cancelled checks. |
|
I think that change is breaking for anyone using turbofish syntax for count at minimum? maybe in practice that's acceptable, unclear |
|
Not sure I 100% understand your meaning about the u24 example. Did it previously read 4 bytes a piece? |
Yes, it did, and the resulting "u24" (which we represent as a u32) can have it's high 8 bits set. So it's not a u24 at all, when going through count_with. edit: if you check out the commit before i fix this, 6b43f69 , you can observe that the new test fails with the equivalent of "unexpected EOF" |
I think this is fine. I’m not worried about the signature change at least. I thought I had done something similar to this in some uncommitted code change but I can’t find it in any of my stashes, so it must have been an experiment I abandoned. This change makes sense to me and seems fine if you want to wrap it up. I’ve pushed a commit to master with updated outputs for the nightly UI tests so that will not fail any more on rebase. |
kitlith
force-pushed
the
correct_count_with
branch
from
c313164 to
656e5eb
Compare
|
... i squashed one too many commits in the rebase. sec. |
Test is written to "what a user would expect", this currently fails. We should either move the optimization to the count helper, or fix it in some other way.
count_with can't make any assumptions about the read function it's been given, unless we find a way to specialize on "read function is "T::read_options" This fixes the test introduced in the previous commit.
Note: this changes the signature of binrw::helpers::count, which may require a semver bump.
kitlith
force-pushed
the
correct_count_with
branch
from
656e5eb to
41fef43
Compare
Splitting this off from #317, as there's more solution space to sort through there, and I think fixing this is more important.
While writing this PR description, I thought of a less contrived example and test where this would break: reading 24 bit integers, which we already provide a helper for.
count_with(N, read_u24)will currently read N u32s, instead of N u24s, and I have updated the test to reflect this.The main thing I'm uncertain about here is the last commit, where I modify the API of
countto make it callable fromVec::read_options. That may be a semver breakage, I'm not sure.Before merging, I figure I'll do an interactive rebase to squash all of the commits prefixed with
fixup!.