Skip to content

Conversation

RaoulSchaffranek
Copy link
Member

@RaoulSchaffranek RaoulSchaffranek commented Aug 28, 2025

This PR adds a new hook implementation for converting byte arrays into hex strings.
This is needed by the kontrol-node to encode memory as JSON strings efficiently.

@RaoulSchaffranek RaoulSchaffranek marked this pull request as draft August 28, 2025 15:20
@RaoulSchaffranek RaoulSchaffranek changed the title WIP: bytes2hexstring bytes2hexstring hook Aug 29, 2025
@RaoulSchaffranek RaoulSchaffranek marked this pull request as ready for review August 29, 2025 10:14
@tothtamas28
Copy link
Contributor

@jberthold, is there a spreadsheet or similar for documenting hook implementations?

- Added code comment
- Renamed hook: bytes2hexstring -> bytes2hex
- Improved test case
- Use safe length to avoid potential out-of-bounds error
Copy link
Contributor

@tothtamas28 tothtamas28 left a comment

Choose a reason for hiding this comment

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

LGTM. I'll leave the final approval to @Robertorosmaninho.

Co-authored-by: Tamás Tóth <[email protected]>
Copy link
Collaborator

@Robertorosmaninho Robertorosmaninho left a comment

Choose a reason for hiding this comment

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

LGTM, just one question:

@@ -9,6 +9,7 @@ expanded_input_file="$(mktemp tmp.in.XXXXXXXXXX)"
parser_file="$(mktemp tmp.parse.XXXXXXXXXX)"
temp_inputs=()

# shellcheck disable=SC2329
Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry, why is this needed here?

Copy link
Member Author

Choose a reason for hiding this comment

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

The shell check CI workflow kept failing, see for example this action: https://github.com/runtimeverification/llvm-backend/actions/runs/17319617915/job/49169763898

If I understand correctly, it's a false positive because the function is used in the following line. So I marked it to skip the check.

It's not related to this PR, I think, but maybe some CI flakiness caused by a shell check update.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Got it! Thanks for clarifying it!

@automergerpr-permission-manager automergerpr-permission-manager bot merged commit fd42f24 into develop Aug 29, 2025
10 checks passed
@automergerpr-permission-manager automergerpr-permission-manager bot deleted the raoul/bytes2hexstring-dev branch August 29, 2025 17:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants