Replace exception-driven associative array key normalization with a non-throwing scan - #561
Conversation
…on-throwing scan AssocArray.toLongKey normalized every key by calling Long.parseLong inside a try/catch, allocating a NumberFormatException (with full stack-trace fill) for every non-integer key: every multidimensional arr[x,y] key and every plain string key, on every get/put/in. Replace it with a hand-rolled scan that accepts and rejects exactly the same inputs as Long.parseLong (radix 10) without ever throwing: same handling of leading +/-, leading zeros, overflow at the Long range boundaries, and non-ASCII digits via Character.digit. Also reuse toLongKey in AssocArray.isIn, which had the same exception-driven pattern. Semantics are unchanged; a parity unit test asserts identical accept/reject/value behavior against Long.parseLong across boundary and Unicode-digit samples. Fixes #559 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f63cecf7b8
ℹ️ 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".
| try { | ||
| return Long.parseLong(key.toString()); | ||
| } catch (Exception e) { // NOPMD - EmptyCatchBlock: intentionally ignored | ||
| final String str = key.toString(); |
There was a problem hiding this comment.
Preserve nonnumeric keys whose toString throws
When an embedding or extension uses an associative array as a key in HashAssocArray, this direct key.toString() call propagates the intentional AwkRuntimeException from HashAssocArray.toString() instead of treating the key as nonnumeric. The previous implementation caught that exception and allowed put, get, remove, and isIn to operate on the object key; AssocArray.mapString() also explicitly supports associative arrays as keys. Catch exceptions from the conversion itself, while retaining the non-throwing scan for ordinary strings.
Useful? React with 👍 / 👎.
An AssocArray used as a key (allowed for extensions) throws AwkRuntimeException from toString(); the previous catch-all swallowed that, so guard the conversion itself while keeping the digit scan non-throwing. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@codex please review again |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b146b30332
ℹ️ 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".
| return null; | ||
| } | ||
| } | ||
| final int len = str.length(); |
There was a problem hiding this comment.
Treat null toString results as nonnumeric
When an embedding uses an object key whose toString() returns null, the old Long.parseLong(key.toString()) path caught the resulting NumberFormatException and treated the key as nonnumeric, but this dereference now throws a NullPointerException. Consequently, HashAssocArray can no longer put, get, or remove that otherwise valid object key; check str for null before scanning it.
Useful? React with 👍 / 👎.
A contract-violating toString() returning null previously fell into the Long.parseLong(null) NumberFormatException path and was treated as non-numeric; keep that behavior with an explicit null check (suppressing the SpotBugs redundant-nullcheck false positive). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@codex please review again |
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep them coming! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
Measures AssocArray.toLongKey across key shapes, HashAssocArray get/put with string keys, and the former exception-driven Long.parseLong implementation as an in-run baseline. Measured on JDK 21 / Windows 11: non-numeric keys drop from ~1100 ns/op (exception-driven) to ~2 ns/op; a 5M-iteration string-keyed arr[k]++ AWK workload runs 2.7x faster end-to-end. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@codex review |
|
JMH results for the new
Non-numeric key normalization drops from ~1100 ns to ~2 ns per call (~550×); numeric keys stay in the same range (17.8 → 20.5 ns). End-to-end AWK workload (5M iterations of
~2.7× faster on string-keyed array workloads. |
|
Codex Review: Didn't find any major issues. Nice work! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
Fixes #559
Problem
AssocArray.toLongKeynormalized every associative-array key by callingLong.parseLonginside a try/catch. For any non-integer key — every multidimensionalarr[x,y]key (SUBSEP-joined) and every plain string key — this allocated aNumberFormatExceptionand filled in its stack trace (~600 ns per call, growing with Java stack depth). This ran on everyget,put, andintest;arr[k]++paid it twice.Fix
toLongKeyis now a hand-rolled, non-throwing scan mirroringLong.parseLong(String)(radix 10) exactly: optional leading+/-, leading zeros, negative accumulation soLong.MIN_VALUEparses, overflow guards at the range boundaries, andCharacter.digitso non-ASCII digits (Arabic-Indic, fullwidth, …) keep behaving exactly as before.AssocArray.isInhad the same exception-driven pattern inline; it now reusestoLongKey.Semantics
Identical by construction, and locked in by a new unit test (
AssocArrayToLongKeyTest): targeted boundary cases plus an exhaustive parity check assertingtoLongKeyaccepts/rejects/produces exactly whatLong.parseLongdoes across ~50 samples (signs, whitespace, leading zeros,Long.MIN_VALUE/MAX_VALUE±1, >19-digit strings, SUBSEP keys, Unicode digits). No entry in behavior-changes.md since user-visible behavior is unchanged.Verification
mvn clean verify: 765 unit tests pass, checkstyle/pmd/spotbugs clean. Compatibility-test results unchanged from the pre-existing baseline.🤖 Generated with Claude Code