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 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.

@urlyy
Copy link
Contributor Author

urlyy commented Nov 30, 2025

Since the tests for RustXlangTest are identical to CPPXlangTest, and sli_int64 has not yet been implemented for cpp, I’ve temporarily commented out the corresponding API test in Rust to allow the CI to pass.

@chaokunyang
Copy link
Collaborator

Let me implement it in another PR for c++

@urlyy
Copy link
Contributor Author

urlyy commented Nov 30, 2025

Let me implement it in another PR for c++

ok😀

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

Labels

None yet

2 participants