Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Conversion builtins for ByStrX to UIntX #907

Merged
merged 14 commits into from Nov 18, 2020
Merged

Conversion builtins for ByStrX to UIntX #907

merged 14 commits into from Nov 18, 2020

Conversation

vaivaswatha
Copy link
Contributor

No description provided.

Comment on lines +65 to +70
let int_bit_width_to_int = function
| Bits32 -> 32
| Bits64 -> 64
| Bits128 -> 128
| Bits256 -> 256

Copy link
Contributor

Choose a reason for hiding this comment

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

We also have the int_width function further below in Types.ml:

  let int_width = function
    | PrimType (Int_typ Bits32) | PrimType (Uint_typ Bits32) -> Some 32
    | PrimType (Int_typ Bits64) | PrimType (Uint_typ Bits64) -> Some 64
    | PrimType (Int_typ Bits128) | PrimType (Uint_typ Bits128) -> Some 128
    | PrimType (Int_typ Bits256) | PrimType (Uint_typ Bits256) -> Some 256
    | _ -> None

Is it possible to reuse int_bit_width_to_int to simplify it into something like this?

  let int_width = function
    | PrimType (Int_typ bits) | PrimType (Uint_typ bits) -> Some (int_bit_width_to_int bits)
    | _ -> None

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Comment on lines 20 to 25
let res7 = builtin to_uint32 zero in
let res8 = builtin to_uint32 one_1 in
let res9 = builtin to_uint32 one_3 in
let res10 = builtin to_uint32 one_4 in
let res11 = builtin to_uint256 one_1 in
let res11 = builtin to_uint256 one_8 in
Copy link
Contributor

Choose a reason for hiding this comment

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

As far as I understand, this PR adds to_uint64 / to_uint128 (and the signed versions too). If this is correct, could we have those tested too, please?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added now for other unsigned ones. I didn't add the builtin itself for signed versions. I'm not convinced we need them. If we do add, we do sign extend if the input bystrx is smaller in size? Not sure. That's why I've left it for now.

@vaivaswatha
Copy link
Contributor Author

@anton-trunov I've addressed your comments, and in addition added builtin strlen for Bystr .

@jjcnn

@anton-trunov
Copy link
Contributor

@vaivaswatha Awesome! Thanks.

@vaivaswatha vaivaswatha merged commit 9d298f6 into master Nov 18, 2020
@vaivaswatha vaivaswatha deleted the polynetwork branch November 18, 2020 15:50
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.

None yet

3 participants