Start cleanup of huf_compress.rs #268
Conversation
…s in huf_compress.rs
Codecov Report❌ Patch coverage is
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 4 files with indirect coverage changes 🚀 New features to boost your workflow:
|
folkertdev
left a comment
folkertdev
left a comment
There was a problem hiding this comment.
Nice, yes this good to review, thanks.
Some minor things, but overall this looks good. Again for easier reviewing, github can't handle force pushes very well, so it's easier if you make changes in later commits. The one exception is maybe assert -> debug_assert which you might be able to amend without horrible rebase conflicts down the line. But a separate commit is fine for that too.
lib/compress/huf_compress.rs
Outdated
| assert!((align & (align - 1)) == 0); /* pow 2 */ | ||
| assert!(align <= HUF_WORKSPACE_MAX_ALIGNMENT); |
There was a problem hiding this comment.
So what's fun is that the assertions in the zstd codebase are actually more like rust's debug_assert!: they don't get compiled into release binaries.
So please convert these to debug_assert!. We only figured this out later, so you might find stray assert!s still.
lib/compress/huf_compress.rs
Outdated
| HUF_flushBits(bitC, kFastFlush); | ||
| n -= kUnroll; | ||
| } | ||
| assert!(n % (2 * kUnroll) == 0); |
There was a problem hiding this comment.
we can use n.is_multiple_of(2 * kUnroll) here.
There was a problem hiding this comment.
n is a c_int which doesn't have is_multiple_of. I didn't want to change the data type in this cleanup
There was a problem hiding this comment.
Right, that wasn't implemented. This is a data point that it maybe should be though. Anyway, that's fine then.
lib/compress/huf_compress.rs
Outdated
|
|
||
| const { | ||
| assert!( | ||
| ::core::mem::size_of::<HUF_compress_tables_t>() + HUF_WORKSPACE_MAX_ALIGNMENT |
There was a problem hiding this comment.
I believe we just have size_of in scope unqualified (this is some sort of version/edition thing). If not, you can import it explicitly at teh top and these lines become a little shorter.
There was a problem hiding this comment.
Done and did same for align_of
|
Awesome, thanks! |
2 participants
Split this into small-ish commits with self explanatory goals.
The first big commit 6e0047e is re-ordering the file to line up with the original C file. This just makes it really easy to compare 1 vs the other.
For assertions, do you want assert! or debug_assert! ?