Skip to content

bytes2hexstring hook#1213

Merged
automergerpr-permission-manager[bot] merged 11 commits into
developfrom
raoul/bytes2hexstring-dev
Aug 29, 2025
Merged

bytes2hexstring hook#1213
automergerpr-permission-manager[bot] merged 11 commits into
developfrom
raoul/bytes2hexstring-dev

Conversation

@RaoulSchaffranek

@RaoulSchaffranek RaoulSchaffranek commented Aug 28, 2025

Copy link
Copy Markdown
Member

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
Comment thread runtime/strings/bytes.cpp Outdated
Comment thread runtime/strings/bytes.cpp Outdated
Comment thread unittests/runtime-strings/bytestest.cpp Outdated
@tothtamas28

Copy link
Copy Markdown
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

@tothtamas28 tothtamas28 left a comment

Copy link
Copy Markdown
Contributor

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.

Comment thread runtime/strings/bytes.cpp Outdated
Co-authored-by: Tamás Tóth <tothtamas28@users.noreply.github.com>

@Robertorosmaninho Robertorosmaninho left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM, just one question:

Comment thread bin/llvm-krun
parser_file="$(mktemp tmp.parse.XXXXXXXXXX)"
temp_inputs=()

# shellcheck disable=SC2329

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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