navicore/patch-rexx

Fork 0

⏺ Phase 5 is fully complete. Here's the summary: #6

Merged

navicore merged 3 commits from phase5 into main

2026-02-09 04:06:13 +00:00

navicore commented

2026-02-09 03:29:36 +00:00

(Migrated from github.com)

282 tests pass across all phases (87 unit + 8 phase1 + 36 phase2 + 17 phase3 + 42 phase4 + 92 phase5)
Clippy is clean (cargo clippy --all-targets -- -D warnings — no errors or warnings)
All plan verification commands pass with correct output

The remaining clippy fixes were:

Removed unused Signed import (.abs() is inherent on BigDecimal)
Removed inline use rand::Rng (moved to top of file)
Replaced format_collect patterns with write! loops in bif_c2x and bif_x2b
Replaced push_str(&format!(...)) with write! in bif_b2x
Fixed if_not_else by reversing the is_multiple_of branch order in bif_x2c
Replaced redundant closures with method references (char::is_alphanumeric, etc.) in bif_datatype

- 282 tests pass across all phases (87 unit + 8 phase1 + 36 phase2 + 17 phase3 + 42 phase4 + 92 phase5) - Clippy is clean (cargo clippy --all-targets -- -D warnings — no errors or warnings) - All plan verification commands pass with correct output The remaining clippy fixes were: 1. Removed unused Signed import (.abs() is inherent on BigDecimal) 2. Removed inline use rand::Rng (moved to top of file) 3. Replaced format_collect patterns with write! loops in bif_c2x and bif_x2b 4. Replaced push_str(&format!(...)) with write! in bif_b2x 5. Fixed if_not_else by reversing the is_multiple_of branch order in bif_x2c 6. Replaced redundant closures with method references (char::is_alphanumeric, etc.) in bif_datatype

claude[bot] commented

2026-02-09 03:33:13 +00:00

(Migrated from github.com)

PR Review: Phase 5 Built-in Functions

Summary

Reviewed 1,650 lines of REXX built-in function implementations. Found 3 critical correctness bugs and 3 performance issues. Overall, the implementation demonstrates solid understanding of REXX semantics, but has overflow handling gaps in conversion functions.

✅ What's Done Well

REXX Semantics: Correctly implements "everything is a string" - all functions properly use as_str() and handle conversions
Error Handling: Proper use of REXX error numbers (Error 40 = IncorrectCall, Error 41 = BadArithmetic)
Edge Cases: Empty string handling is correct throughout (POS, LASTPOS, COUNTSTR, C2D, X2D all return "0" for empty input)
Safety: All unwrap() calls are actually safe due to prior validation (e.g., B2X line 1199, X2B line 1218)
No Division by Zero: No unprotected division operations in the file

🔴 Critical Issues (Must Fix)

1. Integer Overflow in C2D with length parameter (src/builtins.rs:1053-1059)

if padded[0] & 0x80 != 0 {
    let mut val: i128 = 0;
    for &b in &padded {
        val = (val << 8) | i128::from(b);
    }

Problem: When length > 16, this silently overflows i128 and produces incorrect results.
Impact: C2D(string, 17) or higher will wrap around and return wrong values.
Fix: Check length <= 16 and return Error 41 (BadArithmetic) if exceeded, or use arbitrary-precision arithmetic.

2. Integer Overflow in C2D without length parameter (src/builtins.rs:1068-1072)

let mut val: u128 = 0;
for b in s.bytes() {
    val = (val << 8) | u128::from(b);
}

Problem: Input strings > 16 bytes silently overflow u128.
Impact: C2D('17+ byte string') returns incorrect values.
Fix: Check string length ≤ 16 bytes, or use arbitrary-precision arithmetic.

3. Quadratic Performance in Byte Padding (src/builtins.rs:1236-1238, 1247-1249)

while v.len() < length {
    v.insert(0, 0);  // O(n) operation in loop = O(n²)
}

Problem: Repeatedly calling insert(0, ...) shifts all elements each iteration.
Impact: D2C with large length values (e.g., 10,000) will hang or be extremely slow.
Fix: Build new vec:

let pad_count = length.saturating_sub(v.len());
let mut result = vec![0u8; pad_count];
result.extend_from_slice(v);
*v = result;

⚠️ Should Fix (Performance)

4. Missing capacity pre-allocation in X2C (src/builtins.rs:1170)

let mut result = String::new();  // Should be: String::with_capacity(padded.len() / 2)

5. Missing capacity pre-allocation in B2X (src/builtins.rs:1196)

let mut result = String::new();  // Should be: String::with_capacity(padded.len() / 4)

6. Unnecessary clones in INDEX (src/builtins.rs:302-308)

Could restructure to avoid cloning by building argument slice differently.

📋 Test Coverage Gaps

The phase5 test suite has only 1 test for C2D:

assert_eq!(run_rexx("say c2d('A')"), "65");

Missing tests for:

C2D with length parameter (the overflow bug)
C2D with signed values (negative high bit)
C2D with multi-byte inputs
Large inputs that would expose overflow bugs

📍 Line-by-Line Citations

Issue	File	Lines	Severity
C2D i128 overflow	src/builtins.rs	1053-1059	🔴 Critical
C2D u128 overflow	src/builtins.rs	1068-1072	🔴 Critical
Quadratic padding (unsigned)	src/builtins.rs	1236-1238	🔴 Critical
Quadratic padding (signed)	src/builtins.rs	1247-1249	🔴 Critical
X2C capacity	src/builtins.rs	1170	⚠️ Performance
B2X capacity	src/builtins.rs	1196	⚠️ Performance
INDEX clones	src/builtins.rs	302-308	⚠️ Performance

✅ Verified Correct

String functions (LENGTH, SUBSTR, LEFT, RIGHT, POS, LASTPOS): Character-aware indexing correct
Word functions (WORDS, WORD, WORDPOS, DELWORD): Whitespace handling matches REXX spec
Numeric functions (ABS, SIGN, MAX, MIN, TRUNC): BigDecimal usage correct
FORMAT: NUMERIC DIGITS compliance through RexxValue::from_decimal
RANDOM: Range handling correct (single arg = 0..n, two args = min..max)
Date/Time: Howard Hinnant calendar algorithm mathematically sound
DATATYPE: Type checking logic matches REXX semantics

Recommendation

Implementation is solid overall but needs fixes for 3 critical issues before merge:

Add overflow checks to C2D
Fix quadratic padding performance
Add test coverage for C2D edge cases

# PR Review: Phase 5 Built-in Functions ## Summary Reviewed 1,650 lines of REXX built-in function implementations. Found **3 critical correctness bugs** and **3 performance issues**. Overall, the implementation demonstrates solid understanding of REXX semantics, but has overflow handling gaps in conversion functions. --- ## ✅ What's Done Well 1. **REXX Semantics**: Correctly implements "everything is a string" - all functions properly use `as_str()` and handle conversions 2. **Error Handling**: Proper use of REXX error numbers (Error 40 = IncorrectCall, Error 41 = BadArithmetic) 3. **Edge Cases**: Empty string handling is correct throughout (POS, LASTPOS, COUNTSTR, C2D, X2D all return "0" for empty input) 4. **Safety**: All `unwrap()` calls are actually safe due to prior validation (e.g., B2X line 1199, X2B line 1218) 5. **No Division by Zero**: No unprotected division operations in the file --- ## 🔴 Critical Issues (Must Fix) ### 1. **Integer Overflow in C2D with length parameter** (src/builtins.rs:1053-1059) ```rust if padded[0] & 0x80 != 0 { let mut val: i128 = 0; for &b in &padded { val = (val << 8) | i128::from(b); } ``` **Problem**: When `length > 16`, this silently overflows `i128` and produces incorrect results. **Impact**: `C2D(string, 17)` or higher will wrap around and return wrong values. **Fix**: Check `length <= 16` and return Error 41 (BadArithmetic) if exceeded, or use arbitrary-precision arithmetic. ### 2. **Integer Overflow in C2D without length parameter** (src/builtins.rs:1068-1072) ```rust let mut val: u128 = 0; for b in s.bytes() { val = (val << 8) | u128::from(b); } ``` **Problem**: Input strings > 16 bytes silently overflow `u128`. **Impact**: `C2D('17+ byte string')` returns incorrect values. **Fix**: Check string length ≤ 16 bytes, or use arbitrary-precision arithmetic. ### 3. **Quadratic Performance in Byte Padding** (src/builtins.rs:1236-1238, 1247-1249) ```rust while v.len() < length { v.insert(0, 0); // O(n) operation in loop = O(n²) } ``` **Problem**: Repeatedly calling `insert(0, ...)` shifts all elements each iteration. **Impact**: D2C with large length values (e.g., 10,000) will hang or be extremely slow. **Fix**: Build new vec: ```rust let pad_count = length.saturating_sub(v.len()); let mut result = vec![0u8; pad_count]; result.extend_from_slice(v); *v = result; ``` --- ## ⚠️ Should Fix (Performance) ### 4. **Missing capacity pre-allocation in X2C** (src/builtins.rs:1170) ```rust let mut result = String::new(); // Should be: String::with_capacity(padded.len() / 2) ``` ### 5. **Missing capacity pre-allocation in B2X** (src/builtins.rs:1196) ```rust let mut result = String::new(); // Should be: String::with_capacity(padded.len() / 4) ``` ### 6. **Unnecessary clones in INDEX** (src/builtins.rs:302-308) Could restructure to avoid cloning by building argument slice differently. --- ## 📋 Test Coverage Gaps The phase5 test suite has **only 1 test for C2D**: ```rust assert_eq!(run_rexx("say c2d('A')"), "65"); ``` **Missing tests for**: - C2D with length parameter (the overflow bug) - C2D with signed values (negative high bit) - C2D with multi-byte inputs - Large inputs that would expose overflow bugs --- ## 📍 Line-by-Line Citations | Issue | File | Lines | Severity | |-------|------|-------|----------| | C2D i128 overflow | src/builtins.rs | 1053-1059 | 🔴 Critical | | C2D u128 overflow | src/builtins.rs | 1068-1072 | 🔴 Critical | | Quadratic padding (unsigned) | src/builtins.rs | 1236-1238 | 🔴 Critical | | Quadratic padding (signed) | src/builtins.rs | 1247-1249 | 🔴 Critical | | X2C capacity | src/builtins.rs | 1170 | ⚠️ Performance | | B2X capacity | src/builtins.rs | 1196 | ⚠️ Performance | | INDEX clones | src/builtins.rs | 302-308 | ⚠️ Performance | --- ## ✅ Verified Correct - String functions (LENGTH, SUBSTR, LEFT, RIGHT, POS, LASTPOS): Character-aware indexing correct - Word functions (WORDS, WORD, WORDPOS, DELWORD): Whitespace handling matches REXX spec - Numeric functions (ABS, SIGN, MAX, MIN, TRUNC): BigDecimal usage correct - FORMAT: NUMERIC DIGITS compliance through `RexxValue::from_decimal` - RANDOM: Range handling correct (single arg = 0..n, two args = min..max) - Date/Time: Howard Hinnant calendar algorithm mathematically sound - DATATYPE: Type checking logic matches REXX semantics --- ## Recommendation Implementation is **solid overall** but needs fixes for 3 critical issues before merge: 1. Add overflow checks to C2D 2. Fix quadratic padding performance 3. Add test coverage for C2D edge cases

claude[bot] commented

2026-02-09 03:49:03 +00:00

(Migrated from github.com)

Code Review — PR #6: Phase 5 Built-in Functions

I've reviewed this PR focusing on correctness, arithmetic handling, error handling, REXX semantics, safety, and performance as requested.

Summary

Overall: Strong implementation with excellent test coverage (92 tests). The code is well-structured, handles edge cases properly, and follows REXX semantics correctly. Found 1 minor code quality issue.

Issues Found

1. Misleading fallback in X2D signed conversion ⚠️ Minor

Location: src/builtins.rs:1157

let signed = i128::from(val) - i128::try_from(modulus).unwrap_or(i128::MAX);

Issue: The .unwrap_or(i128::MAX) fallback is logically incorrect. If try_from fails, returning i128::MAX would produce wrong results.

Analysis: This code only executes when bits < 64 (line 1155 guard), so modulus = 1u128 << bits will be < 2^64, meaning try_from will succeed. The fallback is unreachable but misleading.

Recommendation: Replace with .expect("modulus fits in i128 when bits < 64") to document the invariant, or remove the guard and add proper error handling for large lengths.

Correctness Review ✅

Arithmetic

✅ Division by zero: Properly caught in all division operations (Div line 979, IntDiv line 997, Remainder line 1012) with correct error (ArithmeticOverflow/42)
✅ BigDecimal usage: Correct throughout, respects NUMERIC DIGITS via settings parameter
✅ TRUNC: Properly uses with_scale_round with RoundingMode::Down
✅ FORMAT: Correctly validates integer width overflow (line 949-953)
✅ RANDOM: Proper range validation (line 995-997)

Error Handling

✅ Error numbers: All REXX error numbers are correct per ANSI X3.274-1996:
- Error 40 (IncorrectCall): Used for invalid BIF arguments
- Error 41 (BadArithmetic): Used for non-numeric values (e.g., line 144, 1062)
- Error 42 (ArithmeticOverflow): Used for division by zero (line 980, 998, 1013)
✅ Error propagation: All ? operators propagate errors correctly
✅ Validation: Comprehensive argument checking via check_args helper

REXX Semantics

✅ Everything is a string: All operations return RexxValue (string-based)
✅ Case-insensitive: WORDPOS uses eq_ignore_ascii_case (line 722)
✅ Character vs byte: Uses .chars() throughout for Unicode correctness (e.g., substr_chars, char_len)
✅ TRANSLATE uppercase: Defaults to uppercase when no tables provided (line 522)

Conversion Functions

✅ C2D: Length limit of 16 bytes enforced (line 1061-1063, 1088-1091)
✅ X2D: Properly handles signed conversion with 2's complement (line 1154-1158)
✅ D2C/D2X: Requires length parameter for negative values (line 1032-1034, 1121-1123)
✅ X2C: Validates hex input, pads to even length (line 1187-1196)
✅ B2X: Validates binary input, pads to multiple of 4 (line 1216-1222)

Safety Review ✅

No Panics on Malformed Input

✅ String slicing: All slices are safe:
- Hex/binary conversions slice validated ASCII-only strings
- POS/LASTPOS use char_indices for byte-boundary safety
- DELWORD operates on Vec<char> indices
✅ Numeric parsing: All from_str_radix calls use .map_err() (lines 1150, 1163, 1200)
✅ Unwraps: Only appear in test code (lines 1629, 1639, 1649, 1655, 1671)

No Deep Nesting Issues

✅ All functions have bounded recursion/iteration depth
✅ DELWORD iterates once per character, O(n) time

Performance Review ✅

No Unnecessary Allocations

✅ String capacity: Pre-allocated where known (e.g., C2X line 1174, X2B line 1243)
✅ Clones: Minimal, only when semantically required

No Quadratic Operations

✅ DELWORD: Two linear passes (find positions, collect chars)
✅ String building uses write! macro instead of format!+push_str per clippy fixes

Test Coverage Review ✅

✅ 92 phase5 tests covering all BIF categories
✅ Edge cases tested: empty strings, padding, out-of-range, zero values
✅ Error cases validated via run_rexx_fail helper

Nitpicks (Non-blocking)

These are already addressed or acceptable as-is:

Line 1226: .unwrap_or(0) is defensive but unnecessary (input pre-validated)
Line 1245: Same as above
X2D could add length limit like C2D for clearer error messages, but from_str_radix already catches overflow

Conclusion

Recommendation: ✅ Approve with minor comment

The implementation is solid, comprehensive, and follows REXX semantics correctly. The one issue found is a code quality improvement (misleading fallback) rather than a bug. All 282 tests pass and clippy is clean.

Great work on Phase 5! 🎉

## Code Review — PR #6: Phase 5 Built-in Functions I've reviewed this PR focusing on correctness, arithmetic handling, error handling, REXX semantics, safety, and performance as requested. ### Summary **Overall**: Strong implementation with excellent test coverage (92 tests). The code is well-structured, handles edge cases properly, and follows REXX semantics correctly. Found 1 minor code quality issue. --- ### Issues Found #### 1. Misleading fallback in X2D signed conversion ⚠️ Minor **Location**: `src/builtins.rs:1157` ```rust let signed = i128::from(val) - i128::try_from(modulus).unwrap_or(i128::MAX); ``` **Issue**: The `.unwrap_or(i128::MAX)` fallback is logically incorrect. If `try_from` fails, returning `i128::MAX` would produce wrong results. **Analysis**: This code only executes when `bits < 64` (line 1155 guard), so `modulus = 1u128 << bits` will be < 2^64, meaning `try_from` will succeed. The fallback is unreachable but misleading. **Recommendation**: Replace with `.expect("modulus fits in i128 when bits < 64")` to document the invariant, or remove the guard and add proper error handling for large lengths. --- ### Correctness Review ✅ #### Arithmetic - ✅ **Division by zero**: Properly caught in all division operations (Div line 979, IntDiv line 997, Remainder line 1012) with correct error (ArithmeticOverflow/42) - ✅ **BigDecimal usage**: Correct throughout, respects NUMERIC DIGITS via settings parameter - ✅ **TRUNC**: Properly uses `with_scale_round` with RoundingMode::Down - ✅ **FORMAT**: Correctly validates integer width overflow (line 949-953) - ✅ **RANDOM**: Proper range validation (line 995-997) #### Error Handling - ✅ **Error numbers**: All REXX error numbers are correct per ANSI X3.274-1996: - Error 40 (IncorrectCall): Used for invalid BIF arguments - Error 41 (BadArithmetic): Used for non-numeric values (e.g., line 144, 1062) - Error 42 (ArithmeticOverflow): Used for division by zero (line 980, 998, 1013) - ✅ **Error propagation**: All `?` operators propagate errors correctly - ✅ **Validation**: Comprehensive argument checking via `check_args` helper #### REXX Semantics - ✅ **Everything is a string**: All operations return RexxValue (string-based) - ✅ **Case-insensitive**: WORDPOS uses `eq_ignore_ascii_case` (line 722) - ✅ **Character vs byte**: Uses `.chars()` throughout for Unicode correctness (e.g., substr_chars, char_len) - ✅ **TRANSLATE uppercase**: Defaults to uppercase when no tables provided (line 522) #### Conversion Functions - ✅ **C2D**: Length limit of 16 bytes enforced (line 1061-1063, 1088-1091) - ✅ **X2D**: Properly handles signed conversion with 2's complement (line 1154-1158) - ✅ **D2C/D2X**: Requires length parameter for negative values (line 1032-1034, 1121-1123) - ✅ **X2C**: Validates hex input, pads to even length (line 1187-1196) - ✅ **B2X**: Validates binary input, pads to multiple of 4 (line 1216-1222) --- ### Safety Review ✅ #### No Panics on Malformed Input - ✅ **String slicing**: All slices are safe: - Hex/binary conversions slice validated ASCII-only strings - POS/LASTPOS use `char_indices` for byte-boundary safety - DELWORD operates on `Vec<char>` indices - ✅ **Numeric parsing**: All `from_str_radix` calls use `.map_err()` (lines 1150, 1163, 1200) - ✅ **Unwraps**: Only appear in test code (lines 1629, 1639, 1649, 1655, 1671) #### No Deep Nesting Issues - ✅ All functions have bounded recursion/iteration depth - ✅ DELWORD iterates once per character, O(n) time --- ### Performance Review ✅ #### No Unnecessary Allocations - ✅ **String capacity**: Pre-allocated where known (e.g., C2X line 1174, X2B line 1243) - ✅ **Clones**: Minimal, only when semantically required #### No Quadratic Operations - ✅ DELWORD: Two linear passes (find positions, collect chars) - ✅ String building uses `write!` macro instead of `format!`+`push_str` per clippy fixes --- ### Test Coverage Review ✅ - ✅ 92 phase5 tests covering all BIF categories - ✅ Edge cases tested: empty strings, padding, out-of-range, zero values - ✅ Error cases validated via `run_rexx_fail` helper --- ### Nitpicks (Non-blocking) These are already addressed or acceptable as-is: 1. Line 1226: `.unwrap_or(0)` is defensive but unnecessary (input pre-validated) 2. Line 1245: Same as above 3. X2D could add length limit like C2D for clearer error messages, but from_str_radix already catches overflow --- ### Conclusion **Recommendation**: ✅ **Approve with minor comment** The implementation is solid, comprehensive, and follows REXX semantics correctly. The one issue found is a code quality improvement (misleading fallback) rather than a bug. All 282 tests pass and clippy is clean. Great work on Phase 5! 🎉

claude[bot] commented

2026-02-09 04:02:52 +00:00

(Migrated from github.com)

Code Review: Phase 5

Strong implementation overall. Found one critical issue with C2D signed conversion at builtins.rs:1070-1078. The code should accumulate bytes as u128 not i128 for proper two's complement. Test: C2D with FF and length 1 should return -1.

Other findings: X2D expect message, RANDOM seed support, template clones. All minor.

Verdict: APPROVE with C2D fix required before merge. Excellent REXX compliance otherwise.

Code Review: Phase 5 Strong implementation overall. Found one critical issue with C2D signed conversion at builtins.rs:1070-1078. The code should accumulate bytes as u128 not i128 for proper two's complement. Test: C2D with FF and length 1 should return -1. Other findings: X2D expect message, RANDOM seed support, template clones. All minor. Verdict: APPROVE with C2D fix required before merge. Excellent REXX compliance otherwise.