Skip to content

Conversation

@richardpringle
Copy link
Member

@richardpringle richardpringle commented Mar 25, 2025

Explain your changes:

  • Manually allocating can potentially side-step a compiler optimization of re-using previous allocations where possible. Also, it's not a good practive to pass in a Vec to a function where all you do with the Vec is call as_slice. A Vec consists of a pointer, a length, and a capacity whereas a slice is only a pointer and a length. Slices should always be prefered when elements aren't being added or removed.

Explain how you tested your changes:

  • I'm hoping for CI to do this for me, but we should really add tests to this crate

TODO: Will update after passing CI

Checklist:

  • Dependency versions are unchanged
    • Notify Velocity team if dependencies must change in CI
  • Modified the current draft of release notes with details on what is completed or incomplete within this project
  • Document code purpose, how to use it
    • Mention expected invariants, implicit constraints
  • Tests were added for the new behavior
    • Document test purpose, significance of failures
    • Test names should reflect their purpose
  • All tests pass (CI will check this if you didn't)
  • Serialized types are in stable-versioned modules
  • Does this close issues? List them
  • Closes #0000

@richardpringle richardpringle requested a review from a team as a code owner March 25, 2025 23:40
@SanabriaRusso
Copy link
Member

!ci-build-me

We were manually creating vectors instead of using iterators and collect
in the kimchi-bindings wasm code. This should be more efficient.
@richardpringle
Copy link
Member Author

!ci-build-me

1 similar comment
@Trivo25
Copy link
Member

Trivo25 commented Mar 31, 2025

!ci-build-me

@richardpringle
Copy link
Member Author

Closing in favour of o1-labs/proof-systems#3152 (since the code moved)

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.

4 participants