Conversation
There was a problem hiding this comment.
Pull request overview
Adds binary formatting support to ZkC printf formatting and adjusts word formatting output for hex/binary.
Changes:
- Introduced
FORMAT_BIN/BinFormat()and added%bsupport inFormat.String(). - Updated
FormatWordto emit0x...for hex and0b...for binary. - Extended the parser to recognize
%binprintfformat strings.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| pkg/zkc/util/formatted_text.go | Adds binary format support and updates hex/binary string rendering in FormatWord. |
| pkg/zkc/compiler/parser/parser.go | Allows %b as a valid printf format specifier during parsing. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
pkg/zkc/util/formatted_text.go
Outdated
| // FORMAT_BIN indicates to format in hexadecimal | ||
| FORMAT_BIN |
There was a problem hiding this comment.
The doc comment for FORMAT_BIN is incorrect: it says the format is hexadecimal, but this constant represents binary output. Please update the comment to refer to binary formatting to avoid confusion for future readers.
pkg/zkc/util/formatted_text.go
Outdated
| return Format{FORMAT_HEX} | ||
| } | ||
|
|
||
| // BinFormat constructs a new hexadecimal format. |
There was a problem hiding this comment.
The BinFormat comment is a copy/paste error (it says it constructs a hexadecimal format). Since BinFormat returns FORMAT_BIN and String() maps that to "%b", the comment should describe constructing a binary format.
| // BinFormat constructs a new hexadecimal format. | |
| // BinFormat constructs a new binary format. |
OlivierBBB
force-pushed
the
1641-minor-improvements-to-printf
branch
from
71652a1 to
c893345
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // FormatWord applies a given format to a given word to generate a formatted string. | ||
| func FormatWord[W word.Word[W]](format Format, word W) string { | ||
| switch format.Code { | ||
| case FORMAT_DEC: | ||
| return word.Text(10) | ||
| case FORMAT_HEX: | ||
| return word.Text(16) | ||
| return "0x" + word.Text(16) | ||
| case FORMAT_BIN: | ||
| return "0b" + word.Text(2) |
There was a problem hiding this comment.
New formatting semantics are introduced here (adding 0x/0b prefixes and base-2 formatting), but there are no Go tests covering FormatWord output. Consider adding a small unit test for util.FormatWord (and/or an integration test that asserts printf output) to lock in the expected strings for %x and %b.
| switch runes[index] { | ||
| case 'd': | ||
| return index + 1, zkc_util.DecimalFormat(), true | ||
| case 'x': | ||
| return index + 1, zkc_util.HexFormat(), true | ||
| case 'b': | ||
| return index + 1, zkc_util.BinFormat(), true | ||
| default: |
There was a problem hiding this comment.
Support for the new '%b' formatting token is added here, but there doesn't appear to be any parser/compile test that exercises it (e.g., a testdata ZkC file containing printf with %b). Adding such a test would prevent regressions where the token is accidentally dropped or changed.
Note
Medium Risk
Changes
printf/debug output formatting semantics by adding%band altering%xto include a0xprefix, which can break consumers or golden-output tests relying on prior string formats.Overview
Extends
printfformat parsing to support%bfor binary output.Updates formatted word rendering to add
0b/0xprefixes for%band%xrespectively, changing the exact text produced by existing hex-formatted prints.Reviewed by Cursor Bugbot for commit c893345. Bugbot is set up for automated code reviews on this repo. Configure here.