Skip to content
This repository has been archived by the owner on Feb 18, 2024. It is now read-only.

Replace fn Offset::is_large() with const Offset::IS_LARGE. #999

Closed
HaoYang670 opened this issue May 21, 2022 · 4 comments · Fixed by #1002
Closed

Replace fn Offset::is_large() with const Offset::IS_LARGE. #999

HaoYang670 opened this issue May 21, 2022 · 4 comments · Fixed by #1002
Labels
backwards-incompatible no-changelog Issues whose changes are covered by a PR and thus should not be shown in the changelog

Comments

@HaoYang670
Copy link
Contributor

Just as what we have done in arrow-rs: apache/arrow-rs#1664, we can use const to represent offset::is_large:

pub trait Offset: super::private::Sealed + Index {
    /// Whether it is `i32` (false) or `i64` (true).
    const IS_LARGE: bool;
}

impl Offset for i32 {
    const IS_LARGE: bool = false;
}

impl Offset for i64 {
    const IS_LARGE: bool = true;
}
@HaoYang670
Copy link
Contributor Author

@jorgecarleitao Do we need this change? Or inline function is good enough?

@jorgecarleitao
Copy link
Owner

Thanks @HaoYang670! It is a constant, so I agree that it is better represented as a const 👍

@HaoYang670
Copy link
Contributor Author

I will file a PR this weekend. It is my pleasure to contribute to Arrow2 !!

@jorgecarleitao
Copy link
Owner

Awesome, thanks a lot 🙇

@jorgecarleitao jorgecarleitao added the no-changelog Issues whose changes are covered by a PR and thus should not be shown in the changelog label May 21, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
backwards-incompatible no-changelog Issues whose changes are covered by a PR and thus should not be shown in the changelog
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants