Skip to content

chore: Outline C.memcpy method#268

Merged
garyschulte merged 1 commit intobesu-eth:mainfrom
kevaundray:kw/memcpy-line-ref
Apr 28, 2025
Merged

chore: Outline C.memcpy method#268
garyschulte merged 1 commit intobesu-eth:mainfrom
kevaundray:kw/memcpy-line-ref

Conversation

@kevaundray
Copy link
Copy Markdown
Contributor

@kevaundray kevaundray commented Mar 19, 2025

This essentially outlines the one liner method into a CopyBytes and PtrOffset method, since it was not clear at first, what was being memcopied

@kevaundray kevaundray marked this pull request as ready for review March 19, 2025 22:13
@matkt matkt requested a review from Copilot April 17, 2025 06:44
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 1 out of 1 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (2)

gnark/gnark-jni/gnark-eip-2537.go:1021

  • [nitpick] Consider adding tests to verify that ptrAtOffset handles pointer arithmetic correctly, especially for edge cases such as offsets close to the buffer boundaries.
func ptrAtOffset(base *C.char, offset int) unsafe.Pointer {

gnark/gnark-jni/gnark-eip-2537.go:1029

  • [nitpick] Consider adding tests for the copyBytes function to cover scenarios such as an empty source slice and verifying the correct placement of bytes at the desired offset.
func copyBytes(dst *C.char, offset int, src []byte) {

@matkt matkt requested a review from garyschulte April 17, 2025 06:44
@garyschulte
Copy link
Copy Markdown
Contributor

LGTM, benchmarks unchanged or better
image

pushed a rebase on 262 for CI reasons

Signed-off-by: Kevaundray Wedderburn <kevtheappdev@gmail.com>
@garyschulte garyschulte enabled auto-merge (squash) April 28, 2025 15:24
@garyschulte garyschulte merged commit 82eeb24 into besu-eth:main Apr 28, 2025
10 checks passed
@garyschulte garyschulte mentioned this pull request Jun 9, 2025
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