Skip to content

Conversation

@urlyy
Copy link
Contributor

@urlyy urlyy commented Nov 15, 2025

What does this PR do?

  1. Add Writer::write_sliint64(...) and Reader::read_sliint64(...) to support SLI_INT64 compression.
  2. Improve cross_language unit tests for Reader::read_i32/i64/sliint64() and Writer::read_i32/i64/sliint64() .

Related issues

#2903

Does this PR introduce any user-facing change?

new methods: Writer::write_sliint64(...) and Reader::read_sliint64(...)

@urlyy urlyy changed the title feat(Rust): support slii64 compression feat(Rust): support sli_int64 compression Nov 15, 2025
#[inline(always)]
pub fn write_sliint64(&mut self, value: i64) {
if value >= HALF_MIN_INT && value <= HALF_MAX_INT {
if (HALF_MIN_INT..=HALF_MAX_INT).contains(&value) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why use this range instead of conditional?

Copy link
Contributor Author

@urlyy urlyy Nov 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the lint error and hint I got:

$ cargo clippy --all-targets --all-features -- -D warnings

error: manual `RangeInclusive::contains` implementation                                                                                                                                                                            
   --> fory-core\src\buffer.rs:182:12
    |
182 |         if value >= HALF_MIN_INT && value <= HALF_MAX_INT {
    |            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use: `(HALF_MIN_INT..=HALF_MAX_INT).contains(&value)`
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#manual_range_contains
    = note: `-D clippy::manual-range-contains` implied by `-D warnings`
    = help: to override `-D warnings` add `#[allow(clippy::manual_range_contains)]`

You can see detail at https://rust-lang.github.io/rust-clippy/master/index.html#manual_range_contains

Maybe more Rust-idiomatic, but actually, I think the condition is still more intuitive.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants