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

std::to_bytes should specify the endian #651

Closed
kevaundray opened this issue Jan 16, 2023 · 5 comments · Fixed by #683
Closed

std::to_bytes should specify the endian #651

kevaundray opened this issue Jan 16, 2023 · 5 comments · Fixed by #683
Labels
compiler frontend `noirc_frontend` crate compiler enhancement New feature or request stdlib Standard library shipped with Noir tooling

Comments

@kevaundray
Copy link
Contributor

Problem

It's not clear what the endian is of std::to_bytes

Solution

Change it to std::to_le_bytes and or optionally add std:to_be_bytes

Alternatives considered

Its possible to just add a comment, but then it becomes cumbersome if we ever add a big_endian flavour, because you will have to_bytes and to_be_bytes

Additional context

@kevaundray kevaundray added enhancement New feature or request stdlib Standard library shipped with Noir tooling compiler compiler frontend `noirc_frontend` crate labels Jan 16, 2023
@kevaundray
Copy link
Contributor Author

@Ethan-000 For this, I think just changing the stdlib function would be a first step and low hanging fruit because then we stop new projects from needing to refactor their code.

Noting that there is an endian already in place -- its just that to_bytes is not explicitly specifying it

@Ethan-000
Copy link
Contributor

Ethan-000 commented Jan 23, 2023

@jfecher wait sir the pr does not fully resolves this issue, we still need a change of acvm to add to_be_bytes I think

@jfecher
Copy link
Contributor

jfecher commented Jan 23, 2023

Indeed, the issue was automatically closed when the PR was merged. I'll re-open it.
Adding to_be_bytes was optional so we could leave it closed but I'll re-open it for discussion on if we want to implement that and keep this open, create another issue for big endian and close this one, or some other option.

@jfecher jfecher reopened this Jan 23, 2023
@Ethan-000
Copy link
Contributor

oh I probably shouldn't mark it as Resolves then, got it

@jfecher
Copy link
Contributor

jfecher commented Jan 25, 2023

After thinking it over some, I'm going to re-resolve this issue as the central issue of specifying endian-ness has been solved. Creating a separate to_be_bytes was optional and we can revisit it at a later time if we determine it is needed.

@jfecher jfecher closed this as completed Jan 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler frontend `noirc_frontend` crate compiler enhancement New feature or request stdlib Standard library shipped with Noir tooling
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants