feat: Added Bits<T> in num module

[akhercha]

Resolves: #4245

Added

• Representation<T> trait & its implementation for scalar types.

Comments

I'm not 100% sure for the naming ; happy to change if you have something better!

---

This change is 

[orizi]

Reviewable status: 0 of 4 files reviewed, 2 unresolved discussions (waiting on @akhercha)

---

corelib/src/lib.cairo line 338 at r1 (raw file):

// Representation.
mod repr;

Find a better name for all.
This sounds like python repr function.

Code quote:

// Representation.
mod repr;

---

corelib/src/repr.cairo line 2 at r1 (raw file):

// === Representation ===

remove all inlines - specifically unrequired, and generally avoid.

[enitrat]

I don't think the name representation carries much semantics on its own. Could we try to find a more meaningful name, like BitLength, BitSize or BitCapacity - or something else (BitRepresentation shortened to BitRepr?)?

[Eikix]

I like what this pr enables us to do, most notably for kakarot in bitshifts,
i'm not aware of how this is achieved in other programming languages, but 'bits_repr' isn't so expressive

[akhercha]

I don't think the name representation carries much semantics on its own. Could we try to find a more meaningful name, like BitLength, BitSize or BitCapacity - or something else (BitRepresentation shortened to BitRepr?)?

Yea this was mainly for """extandability""", I had in mind that more "representations" (kinda means nothing yea) could have be added to it later if needed.

As you suggested it would be something like:

trait Capacity<T> {
    fn bits() -> T;                                    // U8Capacity::bits()
    fn bits_capacity(self: @T) -> T;    // 1_u8::bits_capacity()
}

or

trait Length<T> {
    fn bits() -> T;                                   // U8Length::bits()
    fn bits_length(self: @T) -> T;       // 1_u8::bits_length()
}

Any preference @enitrat ? Or I can keep Representation & just change the methods names.

[Eikix]

I don't think the name representation carries much semantics on its own. Could we try to find a more meaningful name, like BitLength, BitSize or BitCapacity - or something else (BitRepresentation shortened to BitRepr?)?

Yea this was mainly for """extandability""", I had in mind that more "representations" (kinda means nothing yea) could have be added to it later if needed.

As you suggested it would be something like:

trait Capacity<T> {
    fn bits() -> T;                                    // U8Capacity::bits()
    fn bits_capacity(self: @T) -> T;    // 1_u8::bits_capacity()
}

or

trait Length<T> {
    fn bits() -> T;                                   // U8Length::bits()
    fn bits_length(self: @T) -> T;       // 1_u8::bits_length()
}

Any preference @enitrat ? Or I can keep Representation & just change the methods names.

I vote for BitSize or BitsLength

[enitrat]

I like both, but it's up to Ori to decide

Could you add an impl for bytes31 @akhercha ? I just encountered a use case where it would be handy :)

[akhercha]

Since Orizi commented on my issue

this is not "size" - this might be "representation bits" - "size" implies the size in memory.

I would go for BitsLength over BitsSize; that would give me:

trait BitsLength<T> {
    fn bits() -> T;
    fn bits_len(self: @T) -> T;
}

[orizi]

Let us move this trait into:
src/num/traits.cairo

we'd have One and Zero there as well.
This qualification would make this much more readable.

Calling it simple Bits under this premise sounds good to me.

also bits() should probably return an index type (usize?) instead of T.

Reviewable status: 0 of 5 files reviewed, 3 unresolved discussions (waiting on @akhercha)

---

corelib/src/bits_length.cairo line 115 at r3 (raw file):

    fn bits_len(self: @felt252) -> felt252 {
        Felt252BitsLength::bits()
    }

This is wrong - should just not be provided for felts.

Code quote:

impl Felt252BitsLength of BitsLength<felt252> {
    fn bits() -> felt252 {
        252
    }
    fn bits_len(self: @felt252) -> felt252 {
        Felt252BitsLength::bits()
    }

[orizi]

Reviewed 2 of 7 files at r4, 1 of 1 files at r5, all commit messages.
Reviewable status: 3 of 8 files reviewed, 5 unresolved discussions (waiting on @akhercha)

---

corelib/src/num/traits.cairo line 5 at r5 (raw file):

trait Bits<T> {
    /// Returns the length in bits of T.
    fn len() -> usize;

this is the correct name - and this is the main required function.

if we don't have a good name for the

Suggestion:

    /// Returns the length in bits of T.
    fn bits() -> usize;

---

corelib/src/num/traits.cairo line 7 at r5 (raw file):

    fn len() -> usize;
    /// Returns the length in bits of self.
    fn bits(self: @T) -> usize;

remove this for the time being - i don't see a particular need for this in the core lib.
adding an outside trait and impl extension providing this should be very easy - but rather not have it in the corelib for now.

Code quote:

    /// Returns the length in bits of self.
    fn bits(self: @T) -> usize;

---

corelib/src/num/traits.cairo line 111 at r5 (raw file):

impl Bytes31Bits of Bits<bytes31> {
    fn len() -> usize {
        31

Suggestion:

248

[orizi]

Reviewed 1 of 5 files at r3, 2 of 7 files at r4, 2 of 2 files at r6, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @akhercha)

a discussion (no related file):
@yuvalsw for 2nd eye.

---

[yuvalsw]

Reviewed 1 of 5 files at r3, 4 of 7 files at r4, 1 of 1 files at r5, 2 of 2 files at r6, all commit messages.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @akhercha)

---

corelib/src/lib.cairo line 338 at r6 (raw file):

// Num
mod num;

Can you please rebase over https://reviewable.io/reviews/starkware-libs/cairo/4254 so we see the incremental diff?

Code quote:

// Num
mod num;

---

corelib/src/num/traits.cairo line 3 at r6 (raw file):

// === Bits ===

trait Bits<T> {

I actually liked BitSize/BitLength better. I think it's more descriptive and thus less confusing.
Between the 2, I prefer "size" as:

1. It's shorter.
2. Most documentation I know talk about size of types, not length (e.g. size_of in c/c++, Sized in Rust etc.).
3. It returns "usize".
4. If later a function with a self parameter for getting the size of its type would be added externally/internally: IMO "length" may sound like the actual length of the data in the current self, rather than the length of the type. "Size" somehow sounds to me more like the size of the type.

Anyway, please also doc the trait, especially following the relatively long naming discussion.

---

corelib/src/num/traits.cairo line 4 at r6 (raw file):

trait Bits<T> {
    /// Returns the bitsgth in bits of T.

typo?

Code quote:

bitsgth

---

corelib/src/num/traits.cairo line 8 at r6 (raw file):

}

impl U8Bits of Bits<u8> {

please move the impls to integer.cairo.

---

corelib/src/test/num_test.cairo line 10 at r6 (raw file):

#[test]
fn test_traits_bits() {
    assert_eq(@(U8Bits::bits()), @8, 'U8 bits != 8');

aren't these parentheses redundant?

Suggestion:

@U8Bits::bits()

[akhercha]

• Moved the trait to num/traits/bit_size.cairo,
• Updated the trait name to BitSize,
• Moved implementations to integer.cairo & bytes_31.cairo

I also rebased to enitrat:feat/zero-one-traits but I probably messed it up, my commit history is destroyed. 😅

[orizi]

Reviewed 1 of 7 files at r9.
Reviewable status: 5 of 15 files reviewed, 8 unresolved discussions (waiting on @akhercha and @yuvalsw)

[yuvalsw]

Reviewed 3 of 7 files at r8, 6 of 7 files at r9, all commit messages.
Reviewable status: 13 of 15 files reviewed, 7 unresolved discussions (waiting on @akhercha and @orizi)

---

corelib/src/bytes_31.cairo line 4 at r9 (raw file):

use option::OptionTrait;
use integer::{u256_from_felt252, u128_safe_divmod, u128_to_felt252};
use num::traits::BitSize;

this also re-exports it from here. Better fully qualify in the usage instead.

---

corelib/src/integer.cairo line 7 at r9 (raw file):

use array::ArrayTrait;
use array::SpanTrait;
use num::traits::{Zero, One, BitSize};

same here, fully qualify instead.

---

corelib/src/integer.cairo line 2390 at r9 (raw file):

}

// Zero trait implementations

please change target branch as well, so we only see the diff of this PR.

[akhercha]

corelib/src/integer.cairo line 2390 at r9 (raw file):

Previously, yuvalsw wrote…

please change target branch as well, so we only see the diff of this PR.

Do you have any guidance for how to do that properly? At this point I'm scared of breaking my branch again.. 😅

[yuvalsw]

Reviewable status: 11 of 15 files reviewed, 7 unresolved discussions (waiting on @akhercha and @orizi)

---

corelib/src/integer.cairo line 2390 at r9 (raw file):

Previously, akhercha (akhercha) wrote…

Do you have any guidance for how to do that properly? At this point I'm scared of breaking my branch again.. 😅

AFAIK changing the target branch doesn't change anything about your branch. It just means that 1. The diff shown is based from the target branch. 2. If you merge, it's merged into this target (which you don't want - you want to have the target branch back to "main" after the other branch is merged and before merging yours).

Basically, you can hit "edit" in the github PR entry and change from main, or you can do it from revieweable by changing the "to" branch in the summary. But I am not able to do it - probably as I don't have access to the branch.
Another option is to wait for Eni's branch to be merged.

[akhercha]

corelib/src/integer.cairo line 2390 at r9 (raw file):

Previously, yuvalsw wrote…

AFAIK changing the target branch doesn't change anything about your branch. It just means that 1. The diff shown is based from the target branch. 2. If you merge, it's merged into this target (which you don't want - you want to have the target branch back to "main" after the other branch is merged and before merging yours).

Basically, you can hit "edit" in the github PR entry and change from main, or you can do it from revieweable by changing the "to" branch in the summary. But I am not able to do it - probably as I don't have access to the branch.
Another option is to wait for Eni's branch to be merged.

Alright, thanks!
Looks like from both UIs I can't set the target to Eni's branch, it's not in the list. I'll just wait for his PR to be merged then!

[yuvalsw]

Thanks. Now that enitrat:feat/zero-one-traits was merged, please rebase back to main.

Reviewed 2 of 2 files at r10, 1 of 1 files at r11, all commit messages.
Reviewable status: 13 of 15 files reviewed, 5 unresolved discussions (waiting on @akhercha and @orizi)

---

corelib/src/integer.cairo line 2390 at r9 (raw file):

Previously, akhercha (akhercha) wrote…

Alright, thanks!
Looks like from both UIs I can't set the target to Eni's branch, it's not in the list. I'll just wait for his PR to be merged then!

Yeah, I also couldn't do it, not sure why or how you should. Anyway, now it's merged. please re-rebase on main.

[akhercha]

corelib/src/integer.cairo line 2390 at r9 (raw file):

Previously, yuvalsw wrote…

Yeah, I also couldn't do it, not sure why or how you should. Anyway, now it's merged. please re-rebase on main.

Done! Should be good, let me know if anything needs to be changed.

[orizi]

Reviewed 3 of 12 files at r12, 5 of 9 files at r13, 4 of 4 files at r14, all commit messages.
Reviewable status: 11 of 15 files reviewed, 3 unresolved discussions (waiting on @akhercha and @yuvalsw)

---

corelib/src/num/traits/bit_size.cairo line 5 at r14 (raw file):

    /// Returns the size in bits of T as usize.
    fn bits() -> usize;
}

i still don't like the discrepancy between function name and trait here.
any good ideas? @yuvalsw
I thought about something like Bits::capacity, and then later Bits::width (which is the actual number of used bits) could be added.
But i'm really not sure

Code quote:

/// Trait used to retrieve the size in bits of a type.
trait BitSize<T> {
    /// Returns the size in bits of T as usize.
    fn bits() -> usize;
}

[yuvalsw]

Reviewable status: 11 of 15 files reviewed, 3 unresolved discussions (waiting on @akhercha and @orizi)

---

corelib/src/num/traits/bit_size.cairo line 5 at r14 (raw file):

Previously, orizi wrote…

i still don't like the discrepancy between function name and trait here.
any good ideas? @yuvalsw
I thought about something like Bits::capacity, and then later Bits::width (which is the actual number of used bits) could be added.
But i'm really not sure

IF we want to add a method later for the actual number of used bits, capacity and size could be used for the function names. These are the terms generally used for such things (e.g. vector in c++).

But - I don't think we would ever want to add such a method. As I understand it, this is mainly aimed for basic types like u8, u16 etc, and I don't see the general use for a method for the actual number of bits used in such a basic type.
With that in mind - unless you think we would want to add such - I think the name size makes sense. If you want the trait name and the method name to be more consistent, I am also OK with changing the method name to size/bit_size. As there is not self parameter here, you must call it explicitly by BitSize::method_name(), so all are valid names IMO.

[yuvalsw]

Reviewed 8 of 9 files at r13, 4 of 4 files at r14, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @akhercha and @orizi)

[orizi]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @akhercha)

---

corelib/src/num/traits/bit_size.cairo line 5 at r14 (raw file):

Previously, yuvalsw wrote…

IF we want to add a method later for the actual number of used bits, capacity and size could be used for the function names. These are the terms generally used for such things (e.g. vector in c++).

But - I don't think we would ever want to add such a method. As I understand it, this is mainly aimed for basic types like u8, u16 etc, and I don't see the general use for a method for the actual number of bits used in such a basic type.
With that in mind - unless you think we would want to add such - I think the name size makes sense. If you want the trait name and the method name to be more consistent, I am also OK with changing the method name to size/bit_size. As there is not self parameter here, you must call it explicitly by BitSize::method_name(), so all are valid names IMO.

the name size by itself is bad - as it is missing all context.
In anycase - resolving - let it be BitSize::bits();

[orizi]

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @akhercha)

[yuvalsw]

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @akhercha)

[orizi]

Reviewed 2 of 9 files at r13.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @akhercha)

---

corelib/src/test/num_test.cairo line 24 at r14 (raw file):

    assert_eq(@I128BitSize::bits(), @128, 'I128 bit size != 128');
    assert_eq(@Bytes31BitSize::bits(), @248, 'Bytes31 bit size != 248');
}

rebase and:

Suggestion:

use core::num::BitSize;

#[test]
fn test_bit_size() {
    assert!(BitSize::<u8>::bits() == 8);
    assert!(BitSize::<u16>::bits() == 16);
    assert!(BitSize::<u32>::bits() == 32);
    assert!(BitSize::<u64>::bits() == 64);
    assert!(BitSize::<u128>::bits() == 128);
    assert!(BitSize::<u256>::bits() == 256);
    assert!(BitSize::<i8>::bits() == 8);
    assert!(BitSize::<i16>::bits() == 16);
    assert!(BitSize::<i32>::bits() == 32);
    assert!(BitSize::<i64>::bits() == 64);
    assert!(BitSize::<i128>::bits() == 128);
    assert!(BitSize::<bytes31>::bits() == 248);
}

[akhercha]

corelib/src/test/num_test.cairo line 24 at r14 (raw file):

Previously, orizi wrote…

rebase and:

Updated!

[orizi]

Reviewed 8 of 8 files at r15, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @akhercha)