Fix the iOS String.format crash and the toString it relied on (issue #5482) - #5510
Conversation
…5482) String.format was a native method whose Objective-C branch formatted a string, threw it away, and returned fromNSString([NSString init]) -- sending init to the NSString class object, which aborts the process with "+[NSString<0x...> init]: cannot init a class object". Every String.format call on iOS killed the app. The C fallback that ran everywhere else did not crash but ignored width and precision, so "%.3f" printed every digit of the double. The Apple branch sits behind #if defined(__APPLE__) && defined(__OBJC__) and parparvm-tests runs on ubuntu-latest, so CI only ever compiled the #else branch and nothing flagged either problem. Formatting is string manipulation, so drop the native entirely and implement it once in java.lang.StringFormatter. One implementation now serves iOS, the JavaScript target and the C fallback, which also makes the Linux CI test meaningful for iOS. Supported conversions are s S b B h H c C d o x X e E f g G n %, with the - + ' ' 0 , ( # flags, width, precision and the %n$ / %< argument selectors. %a (hexadecimal float) and %t (date and time) are not implemented and raise UnknownFormatConversionException rather than producing something wrong. A malformed format string now raises the java.util exception the JVM raises (the ten missing classes are added here) instead of taking the process down, which is what the reporter asked for. Two further defects surfaced once the output could be compared with a JVM: - Double.toString and Float.toString were badly non-conforming. They asked snprintf for a fixed "%f" (six decimals) in the plain range and "%1.20E" (twenty-one significant digits) in the scientific range, so 1.0/3.0 rendered as "0.333333" instead of "0.3333333333333333" and 1e30 rendered as "1.00000000000000001988E30" instead of "1.0E30". That affected every concatenation of a double, not just formatting. Replaced with a search for the shortest rendering that round trips, which is what the specification asks for. - Math.abs(-0.0) returned -0.0, because "a < 0" is false for negative zero. Now fabs/fabsf. Coverage: StringFormatIntegrationTest and GetClassIntegrationTest run the same program on a real JVM and under ParparVM and diff it case by case, so the expectations are the JDK's rather than hand written. On macOS the harness compiles nativeMethods.m as Objective-C, so these exercise the branch that was crashing. Off-line, the implementation was diffed against the JDK over 384k value cases, 300k randomly assembled format strings and 399k random double and float bit patterns with no divergence. GetClassIntegrationTest also covers the reporter's other suspicion, that getClass() was returning null. It does not: getClassImpl cannot return null for a non-null receiver, and Class identity, hashing, string conversion and use as a HashMap key all match the JVM under allocation churn. The test pins that. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🟡 Not ready to approve
The new formatter has a confirmed edge-case bug for explicit argument index 0$ handling and there is a public JavaAPI compatibility issue in IllegalFormatException constructor visibility.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
This PR fixes the iOS hard-crash in String.format() (issue #5482) by removing the native implementation and replacing it with a shared Java formatter (java.lang.StringFormatter) that is exercised by new differential integration tests. It also corrects ParparVM’s Double.toString/Float.toString conformance and fixes Math.abs(-0.0) to match JVM behavior.
Changes:
- Replace native
String.format()with a Java implementation (StringFormatter) used across targets (iOS/JS/C fallback). - Fix ParparVM floating-to-string rendering to produce the shortest round-tripping decimal and correct
Math.abs()for negative zero. - Add differential integration tests (JVM vs ParparVM) for
String.format()output andgetClass()/Classbehavior under allocation churn.
File summaries
| File | Description |
|---|---|
| vm/tests/src/test/resources/com/codename1/tools/translator/StringFormatApp.java | Test program emitting per-case String.format() results for line-by-line diffing. |
| vm/tests/src/test/resources/com/codename1/tools/translator/GetClassApp.java | Test program pinning getClass() and Class identity/hash/toString under churn. |
| vm/tests/src/test/java/com/codename1/tools/translator/StringFormatIntegrationTest.java | Differential JVM vs ParparVM integration test for formatting output and unsupported conversions. |
| vm/tests/src/test/java/com/codename1/tools/translator/GetClassIntegrationTest.java | Differential JVM vs ParparVM integration test for getClass() and Class map-key behavior. |
| vm/JavaAPI/src/java/util/UnknownFormatConversionException.java | Adds missing java.util exception used by formatter error paths. |
| vm/JavaAPI/src/java/util/MissingFormatWidthException.java | Adds missing java.util exception used by formatter width validation. |
| vm/JavaAPI/src/java/util/MissingFormatArgumentException.java | Adds missing java.util exception used for missing args/indexes. |
| vm/JavaAPI/src/java/util/IllegalFormatWidthException.java | Adds missing java.util exception used for illegal width handling. |
| vm/JavaAPI/src/java/util/IllegalFormatPrecisionException.java | Adds missing java.util exception used for illegal precision handling. |
| vm/JavaAPI/src/java/util/IllegalFormatFlagsException.java | Adds missing java.util exception used for illegal flag combinations. |
| vm/JavaAPI/src/java/util/IllegalFormatException.java | Adds missing java.util base exception type for formatter-related unchecked errors. |
| vm/JavaAPI/src/java/util/IllegalFormatConversionException.java | Adds missing java.util exception used for wrong argument type per conversion. |
| vm/JavaAPI/src/java/util/IllegalFormatCodePointException.java | Adds missing java.util exception for invalid code points in %c/%C. |
| vm/JavaAPI/src/java/util/FormatFlagsConversionMismatchException.java | Adds missing java.util exception for flag/conversion mismatches. |
| vm/JavaAPI/src/java/util/DuplicateFormatFlagsException.java | Adds missing java.util exception for duplicated flags. |
| vm/JavaAPI/src/java/lang/StringFormatter.java | New shared Java implementation of String.format() logic (parsing, conversions, rounding). |
| vm/JavaAPI/src/java/lang/String.java | Switches String.format() from native to StringFormatter.format(). |
| vm/ByteCodeTranslator/src/nativeMethods.m | Fixes Double.toString/Float.toString conformance and Math.abs() negative-zero behavior; removes native String.format. |
| vm/ByteCodeTranslator/src/javascript/parparvm_runtime.js | Removes JavaScript native String.format() binding (now handled in Java). |
| vm/ByteCodeTranslator/src/com/codename1/tools/translator/JavascriptNativeRegistry.java | Removes String.format from JS native registry list. |
Review details
- Files reviewed: 20/20 changed files
- Comments generated: 2
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d24938b462
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
✅ ByteCodeTranslator Quality ReportTest & Coverage
Benchmark Results
Static Analysis
Generated automatically by the PR CI workflow. |
✅ Continuous Quality ReportTest & Coverage
Static Analysis
Generated automatically by the PR CI workflow. |
Cloudflare Preview
|
|
Compared 146 screenshots: 146 matched. Benchmark ResultsDetailed Performance Metrics
|
|
Compared 146 screenshots: 146 matched. Benchmark ResultsDetailed Performance Metrics
|
|
Compared 147 screenshots: 147 matched. |
|
Compared 147 screenshots: 147 matched. |
|
Compared 181 screenshots: 181 matched. |
|
Compared 146 screenshots: 146 matched. Benchmark ResultsDetailed Performance Metrics
|
|
Compared 144 screenshots: 144 matched. |
|
Compared 217 screenshots: 217 matched. |
|
Compared 143 screenshots: 143 matched. Benchmark Results
Build and Run Timing
Detailed Performance Metrics
|
…headers Three review findings: - "%0$s" recorded argIndex == 0, which the parser then treated as "no explicit index given" and satisfied from the next sequential argument. Argument indexes are 1-based, so this now raises IllegalFormatArgumentIndexException the way JDK 16 and later do. JDK 11 still accepts index zero, so the case is asserted against ParparVM alone rather than through the shared diff. - A '.' with no digits after it defaulted the precision to zero, so "%.s" quietly produced an empty string. Every supported JDK rejects it with UnknownFormatConversionException naming '.' as the conversion; so do we now. - The four new test files were missing the Codename One GPLv2 + Classpath Exception header, which failed check-copyright-headers. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Compared 149 screenshots: 149 matched. Benchmark Results
Build and Run Timing
Detailed Performance Metrics
|
There was a problem hiding this comment.
🟡 Not ready to approve
There are correctness/test-stability issues in the new formatter parsing and integration test setup that should be addressed to avoid divergent behavior and flaky results.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (2)
vm/JavaAPI/src/java/lang/StringFormatter.java:154
- The parser currently allows combining an explicit argument index (e.g. "%2$") with the previous-argument flag ('<'). That combination is not meaningful and the current logic will silently prioritize
previousand ignore the explicit index (or throwMissingFormatArgumentExceptionif it’s the first specifier), diverging fromjava.util.Formatter’s behavior for invalid format strings. Consider rejecting '<' when an explicit argument index was already parsed for this specifier.
} else if (f == '<') {
previous = true;
pos++;
continue;
vm/tests/src/test/resources/com/codename1/tools/translator/StringFormatApp.java:244
- This integration test derives its expected output from a JVM run of
String.format(), but several cases depend on the JVM default locale (e.g.%,d,%,.2f, and%Suppercasing). Without pinning the default locale, the diff can become environment-dependent and fail on machines with non-"en_US" defaults even if ParparVM is correct. Consider setting the default locale explicitly at the start ofmain().
public static void main(String[] args) {
strings();
integers();
floats();
failures();
- Files reviewed: 21/21 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
|
Compared 148 screenshots: 148 matched. Benchmark Results
Detailed Performance Metrics
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2e37e4b8ef
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Two more review findings, both confirmed against the JDK first: - format(fmt, (Object[]) null) is not an empty argument list. The JVM skips the bounds checks and hands every specifier a null, so "%s %s" renders "null null" where this threw MissingFormatArgumentException. "%<" is the one exception: it reuses the previous argument, so it still requires that one existed, which is what the parser fuzz caught after the first attempt made null unconditional. - '<' was consumed outside the duplicate-flag check, so "%s %<<s" quietly rendered "a a" instead of raising DuplicateFormatFlagsException. It is now a flag bit like every other, which also makes it participate in the %% and %n flag validation without the special case that was there before. Both behaviours are identical on JDK 11, 17 and 25, so the new cases go through the shared JVM-vs-ParparVM diff rather than being asserted one-sided. The parser sweep now also generates null argument arrays and doubled flags; divergence is still zero apart from the documented %a and %t gap. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🟡 Not ready to approve
The new differential test currently depends on the host default Locale and the public String.format() javadoc doesn’t document the locale-independent behavior, both of which can cause avoidable instability/confusion.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (2)
vm/tests/src/test/java/com/codename1/tools/translator/StringFormatIntegrationTest.java:202
- The JVM side of this differential test depends on the host default Locale for grouping/decimal separators. Since the ParparVM implementation is documented as locale-independent (always ',' grouping and '.' decimal), this test can become flaky or fail on machines with a non-English default locale. Consider forcing a known locale for the JVM run via system properties so the expected output matches the intended locale-independent behavior consistently.
ProcessBuilder pb = new ProcessBuilder(
javaExe,
"-cp",
classesDir + System.getProperty("path.separator") + javaApiDir,
"StringFormatApp"
vm/JavaAPI/src/java/lang/String.java:1085
- The public String.format() javadoc here doesn’t mention that this implementation is locale-independent (fixed '.' decimal separator and ',' grouping, and %n emits '\n'), which is a behavioral difference from the JDK that callers may rely on. Since this is a public API entry point, consider documenting the locale behavior (and that unsupported conversions like %a/%t throw UnknownFormatConversionException) here rather than only in the internal StringFormatter class.
/**
* Returns a formatted string using the specified format string and arguments.
* Supports the {@code s b h c d o x e f g n %} conversions (and their uppercase
* variants) with the {@code - + ' ' 0 , ( #} flags, width, precision, and the
* {@code %n$} / {@code %<} argument selectors.
- Files reviewed: 21/21 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
The vm/tests differential covers ParparVM's C and Objective-C target, but that is not the path a shipping app takes: the iOS build translates the core out of the bundled iOSPort.jar rather than out of the reactor, JavaSE and Android run their own java.util.Formatter, and the JavaScript port runs the translated Java through its own runtime. Those are four different code paths reaching the same API, and nothing was asserting that they agree. StringFormatTest runs the same 124 expectations on every port the suite covers: iOS GL and Metal, tvOS, watchOS, Android, JavaScript, mac native, Linux, Windows and the JavaSE simulator. It takes no screenshot; it is a pure assertion test in the shape of the existing FloatingToStringTest. Every expected value was produced by a real Java SE java.util.Formatter rather than written by hand. The generator is checked in as scripts/hellocodenameone/tools/generate-string-format-cases.java so the table can be regenerated and audited, and it emits a byte-identical table on JDK 8, 11, 17, 21 and 25 -- so this pins behaviour that does not drift with the JDK the suite happens to build against. Coverage includes the conversions, flags, widths and precisions an app actually uses, the HALF_UP rounding cases where Java disagrees with C printf, and the four malformed-format cases found in review on this branch (empty precision, repeated flags, grouping on hex, zero padding on a string). Three things are deliberately excluded because they are genuinely not consistent across these runtimes, rather than papered over: - %a and %t, which ParparVM does not implement. - %0$s, accepted before JDK 16 and rejected from 16 on. - a pinned value for %n, since the JDK emits the platform line separator and that is "\r\n" on a Windows JVM. The test asserts it is a line separator instead. JavaSE and Android format through the default locale, so the test probes the platform's decimal and grouping separators rather than assuming them, and fails with a specific message if the platform also localises the digits. Registered in Cn1ssDeviceRunner and the java-standard-apis feature group. The stored per-port reports get a not-run entry, which the next master publish replaces with the real result; the pinned test count moves 170 -> 171. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🟡 Not ready to approve
The new formatter currently rejects the standard %F conversion and the conformance sweep omits %F, leaving a spec-compatibility gap untested.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (2)
vm/JavaAPI/src/java/lang/StringFormatter.java:250
- StringFormatter currently rejects the standard Java Formatter conversion '%F' (uppercase floating-point). In java.util.Formatter, both %f and %F are valid; %F mainly affects the casing of NaN/Infinity. Because validateSpecifier only whitelists uppercase conversions in "SBHCXEG", any %F format string will incorrectly throw UnknownFormatConversionException.
private static char validateSpecifier(char conversion, boolean upper, int flags,
int width, int precision) {
if (upper && "SBHCXEG".indexOf(conversion) < 0) {
// 'D' and 'O' have no uppercase form in java.util.Formatter.
throw new UnknownFormatConversionException(String.valueOf(conversion));
}
vm/tests/src/test/java/com/codename1/tools/translator/StringFormatConformanceTest.java:79
- The exhaustive conformance sweep doesn't include the standard '%F' conversion, so it won't catch regressions/bugs in uppercase floating-point formatting (or the current rejection of %F). Since this test is meant to cover all supported conversions against the JDK, add "F" to the CONVERSIONS list.
- Files reviewed: 38/38 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a1208ab9a6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…eep first
The oversized-field assertions I added in the previous commit failed vm-tests,
and that failure was mine. A width too large for an int is silently dropped up
to at least Java 11 and rejected from Java 17 on:
"%2147483648d" -> "1" on JDK 8 and 11
"%2147483648d" -> IllegalFormatWidthException on JDK 17, 21 and 25
The sweep compares against whichever JDK runs it, CI runs it on 8, and this
formatter follows the modern behaviour, so the comparison could not hold there.
Those specs are now gated on a runtime probe, the way the "%5%" width difference
already was. The large-but-representable cases -- "%1000001d" and friends, which
are the ones the review comment was actually about -- are still asserted
unconditionally, because every JDK agrees on them.
The probe needed a guard of its own: on a modern JDK it throws rather than
returning a value, which the cross-JDK check caught.
Also moved the oversized block after the main sweep assertion. It ran before it,
so a failure there masked the result of the million-case sweep entirely -- the CI
run that failed never reported whether the sweep itself was clean.
Verified per JDK rather than assumed: sweep and oversized checks are both clean
on 8, 11, 17, 21 and 25, and the test passes under Maven on JDK 8, which is the
configuration that failed. Full ParparVM suite 412 green.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🟡 Human review recommended
It replaces core formatting behavior across multiple runtimes and adjusts native numeric rendering, which is high-impact and warrants final human validation despite the strong test additions.
Review details
- Files reviewed: 38/38 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 15faf83a9f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…ng the test My hand edits to these files are gone. They are machine generated snapshots of a single run, pinned by commit and run_url, and I had no business writing statuses into them by hand -- first not-run, which inflated every port's count from 5 to 6, then pass, which attributed a result to July runs that could not have produced it. These are the reports this PR's own CI generated, at commit 12ecf37, which contains and registers StringFormatTest. Nothing here is written by hand. java-standard-apis now executes everywhere: FloatingToString, Java17, NanoTimeApi, StreamApi, StringApi, StringFormat and TimeApi all report a real result on all eleven ports rather than not-run. not-run drops from 5 to 0 on the device ports, and to 3 on linux and windows where three tests genuinely do not run. The reports also carry seven failures on linux and windows -- AudioMixerApi, Crypto, FileSystemStorageOpenInputStreamMissing, SurfacesPublish, TimeApi and friends. Those are not from this branch. Master's own run today at 9783b6c records the identical seven on the same ports, and the stored snapshots simply predate them by three weeks. They arrive here because the reports are being refreshed, not because anything in this PR caused them. Contract validate and the contract unit tests are green. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🟡 Human review recommended
It changes core formatting and floating-point/string rendering behavior across multiple runtimes (JavaAPI, ParparVM native, and JS), which is high-impact and warrants final human review despite the added conformance tests.
Review details
- Files reviewed: 38/38 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1d675aa4f1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…ption metadata
Four review findings, all confirmed against the JVM first.
The formatter validated one specifier at a time as it emitted, so a broken
specifier late in a format was masked by a missing argument early in it:
String.format("%s %q") with no arguments reported the missing argument where the
JVM reports the unknown conversion. Parsing is now a shared parseSpec and runs
twice -- once to validate every specifier, then once to format -- which is what
the JVM does. One Spec object per call, no per-specifier garbage.
That exposed a second ordering rule. The JVM matches the shape of a specifier
before it judges the parts, so "%,," is an unknown conversion rather than a
duplicate flag: there is no conversion character for those flags to belong to,
and the character reported is the one right after the '%'. Once the shape is
complete the parts are judged index, then duplicate flags, then width, then
precision. All of that was probed rather than guessed:
"%,," -> UnknownFormatConversionException conv=,
"%1$,," -> UnknownFormatConversionException conv=1
"%,,d" -> DuplicateFormatFlagsException
"%0$,,d" -> IllegalFormatArgumentIndexException
"%,,2147483648d" -> DuplicateFormatFlagsException
A supported uppercase conversion carried its case into exception metadata: %G
with an Integer produced getConversion() == 'G' where the JVM reports 'g',
because the JVM lowercases a known conversion and keeps the case as a flag. An
unknown conversion keeps its case, and still does. The parsed lowercase form is
now what reaches the exceptions.
MissingFormatWidthException dropped an explicit argument index from the specifier
it reports. The JVM rebuilds it with the flags ahead of the index -- "%1$-s" is
reported as "%-1$s" and "%2$0d" as "%02$d" -- which this now does too.
toString() may legally return null. An unadorned %s appends it and so renders
"null"; the JVM only tolerates it there, and still fails on "%10s", "%.2s" and
"%S". That distinction is preserved rather than blanket-normalised.
Measured after the change, all against the JVM: the exhaustive single-specifier
sweep is 1,011,712 cases with zero mismatches; the 300k random multi-specifier
fuzz has zero value divergences and zero non-%t divergences, down from 427 for
the shape rule and 37,524 before whole-format validation. Everything left
involves %t, which is documented as unimplemented.
Full ParparVM suite 412 green, conformance green on JDK 8 as well as the default,
device test clean.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🟡 Not ready to approve
The updated native floating-to-string rewrite helper has an out-of-bounds write risk due to unconditional writes that bypass the intended at < limit bounds checks.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
- Files reviewed: 38/38 changed files
- Comments generated: 1
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
Review is right, and AddressSanitizer confirms it: cn1JavaFloatingText could write
one byte past its buffer. In the plain branch the digit loop stops at
limit = outSize - 2, and the '.' and trailing '0' that follow it were
unconditional, so at could reach outSize - 1 before the final out[at] = 0 landed
on out[outSize]. My comment claiming every write there was bounded was wrong.
==ERROR: AddressSanitizer: stack-buffer-overflow
WRITE of size 1 ... in cn1JavaFloatingText
Not reachable from Double.toString or Float.toString, which only take that branch
for 1e-3 <= |d| < 1e7 and so pass an exponent of at most 6. Reachable if anything
else ever calls the helper, which is exactly what the comment was there to
promise.
Rather than add checks in front of the two writes review pointed at, every append
now goes through cn1AppendChar, which refuses to write without room for the
character and the terminator. Boundedness is a property of the helper instead of
an argument about the largest exponent that can get here.
Verified under ASan and UBSan with an exponent large enough to fill the buffer, in
both branches, plus an 8 byte and a 1 byte buffer: clean, and the old code faults
on the first of those. Output is unchanged where it matters -- the curated
toString cases still match the JDK exactly and the 399110 random double and float
bit patterns still round trip with zero mismatches. Full ParparVM suite 412 green.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🔵 Human review recommended
It changes core formatting and floating-point rendering across ParparVM/native and JavaScript runtimes, which is high-impact and warrants final human review despite the strong test additions.
Review details
Suppressed comments (1)
vm/tests/src/test/java/com/codename1/tools/translator/StringFormatConformanceTest.java:272
- URLClassLoader is created but never closed, which can leak file handles (and can be problematic on Windows if any of the generated classes/resources are later cleaned up). Since the loader is only used to load the relocated formatter and obtain a Method, it can be safely wrapped in try-with-resources.
- Files reviewed: 38/38 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0fac2b68ba
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Review is right again, and this is the same shape rule as last time, one layer
deeper. I had made the parser decide the shape was complete as soon as any
character sat in the conversion slot. The JVM is stricter: that slot only accepts
a letter or '%', so "%00." never forms a specifier at all.
"%00." -> UnknownFormatConversionException conv='0' (was DuplicateFormatFlags)
"%0." -> UnknownFormatConversionException conv='0'
"%5.2" -> UnknownFormatConversionException conv='5'
"%,,." -> UnknownFormatConversionException conv=','
"%5!" -> UnknownFormatConversionException conv='5'
"%_d" -> UnknownFormatConversionException conv='_'
"%00%" -> DuplicateFormatFlagsException ('%' IS a conversion)
"%00$d" -> IllegalFormatArgumentIndexException (shape fine, index judged)
Note the reported character is the one right after the '%', not the offending one,
and that "%00%" still reports the duplicate flag -- the rule is about the shape
matching, not about giving up early.
Also closes the URLClassLoader in StringFormatConformanceTest, which review raised
separately. It needs to stay open for the whole sweep rather than just long enough
to resolve the method, because the relocated exception classes load lazily the
first time a malformed specifier throws one, so the sweep now runs inside the
try-with-resources.
Measured after the change: exhaustive single-specifier sweep 1,011,712 cases with
zero mismatches; 300k random multi-specifier formats with zero value divergences
and zero divergences that do not involve %t. Full ParparVM suite 412 green,
conformance green on JDK 8 as well, device test clean.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🟡 Changes recommended
The new differential test output encoding is OS-dependent for %n (CRLF vs LF), which can cause the integration test to fail on Windows unless normalized.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (1)
vm/tests/src/test/resources/com/codename1/tools/translator/StringFormatApp.java:57
- escape() currently emits "\r\n" for Windows CRLF line separators (e.g., from "%n"), while the ParparVM implementation always emits "\n". This makes the differential output OS-dependent and can cause the integration test to fail on Windows runners. Consider normalizing CRLF to a single "\n" escape during rendering so the diff is portable.
- Files reviewed: 38/38 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a698ab806b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| throw new MissingFormatWidthException( | ||
| "%" + flagString(spec.flags) + index + spec.conversion); |
There was a problem hiding this comment.
When a width-dependent flag is combined with a precision but no width, this reconstructed MissingFormatWidthException specifier drops the parsed precision. For example, the JDK reports String.format("%-.2s", "abc") via getFormatSpecifier() as %-.2s and %1$0.2f as %01$.2f, but this path emits %-s/%01$f, so callers or logs that inspect the exception message/getter lose part of the malformed format. Append the precision field when rebuilding this specifier.
Useful? React with 👍 / 👎.
Ten conflicts, all in docs/website/data/port_status_reports/*.json -- the checked-in fallback snapshots. Master's are newer (2026-08-04 against this branch's 2026-07-30) and describe the same contract: the manifest and the runner's registered test list are identical between the two, so master's reports carry no drift and are simply better data. Taken wholesale. test_publishable_matches_every_report_the_site_serves is what makes that safe to assert rather than assume -- it validates every checked-in report against the current contract and demands no drift and nothing malformed. 27 gate tests pass. Verified on the merged tree: 4754 core tests with SpotBugs, the suite builds, and the four ParparVM parity suites (TimeZoneOffsetFrame, CustomTimeZoneId, CharacterLatin1Type, TimeApiIntegration) all pass. Those four failed at first with a duplicate java_lang_String_format symbol, which was purely local: my ByteCodeTranslator target/classes still held an Aug-2 copy of nativeMethods.m carrying the native String.format that master's #5510 removed in favour of the Java StringFormatter. Neither master's source nor the merged source has it; rebuilding the translator cleared it. Nothing to fix in the tree. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Fixes #5482.
The crash
String.formatwas a native method. Its Objective-C branch formatted a string, threw it away, and returnedfromNSString([NSString init])-- sendinginitto theNSStringclass object:That aborts the process with
+[NSString<0x...> init]: cannot init a class object, which is exactly the termination the reporter pasted. EveryString.formatcall on iOS killed the app.The C fallback used everywhere else did not crash, but ignored width and precision entirely, so
"%.3f"printed every digit of the double and"%.2e"printed%.followed by the whole number and a literal2e.Neither was caught because the Apple branch is behind
#if defined(__APPLE__) && defined(__OBJC__)andparparvm-testsruns onubuntu-latest, so CI only ever compiled the#elsebranch.The fix
Formatting is string manipulation, so the native is gone and the work happens once in
java.lang.StringFormatter. A single implementation now serves iOS, the JavaScript target and the C fallback -- which also means the Linux CI test is finally meaningful for iOS.Supported:
s S b B h H c C d o x X e E f g G n %, the- + ' ' 0 , ( #flags, width, precision, and the%n$/%<argument selectors. Rendering is locale independent.%a(hexadecimal float) and%t(date and time) are not implemented; they raiseUnknownFormatConversionExceptionrather than producing something wrong. Both are documented in the class javadoc and pinned by the test.A malformed format string now raises the
java.utilexception the JVM raises -- the ten missing exception classes are added here -- instead of taking the process down. That was the reporter's closing request: "it should have been a trapped error, presented as some kind of a runtime fault, rather than a hard crash."Two further defects the new test exposed
Once the output could be diffed against a JVM, two unrelated ParparVM bugs failed the test:
Double.toString/Float.toStringwere badly non-conforming. They asked snprintf for a fixed"%f"(six decimals) in the plain range and"%1.20E"(twenty-one significant digits) in the scientific range.1.0/3.0rendered as"0.333333"instead of"0.3333333333333333";1e30rendered as"1.00000000000000001988E30"instead of"1.0E30". This hit every concatenation of a double on iOS, not just formatting. Replaced with a binary search for the shortest rendering that round trips, which is what the specification asks for.Math.abs(-0.0)returned-0.0, becausea < 0is false for negative zero. Nowfabs/fabsf.The reporter's other suspicion
He also suspected
getClass()was returning null. It is not:getClassImplcannot return null for a non-null receiver -- it hands back&ClazzClazzwhen the class reference is absent.GetClassIntegrationTestreproduces his exact shape (interface-typed reference,getClass()used as aHashMapkey, the"class is " + clconcatenation that printednull) and re-checks every invariant across 200k allocations of churn. Class identity, hashing, string conversion and map lookup all match the JVM. That symptom was downstream of the process already being wrecked.Coverage
StringFormatIntegrationTestandGetClassIntegrationTestrun the same program on a real JVM and under ParparVM and diff it case by case, so the expectations are the JDK's rather than hand written. On macOS the harness compilesnativeMethods.mas Objective-C, so these exercise the branch that was crashing.Off-line, the implementation was diffed against the JDK over:
%a/%tgap)toStringJDK 11 and 17 differ from the last row on ~5.6% of random doubles because they predate JDK-4511638; the values used in the committed test are byte-identical across JDK 11, 17 and 25.
Full ParparVM suite: 411 tests, 0 failures.
🤖 Generated with Claude Code