25662: Adds simplify opcode, improves consistency of subtraction and division opcodes, MINOR - #634
Conversation
…malgam into 25662-simplify-opcode2
howso-mario
left a comment
There was a problem hiding this comment.
Requesting changes on current head 1a840713 after a fresh full-diff review against current main. I found three focused issues in the new simplify/sort behavior; the inline comments include exact reproducers and patch/test shapes.
Verification: the CI-faithful amd64-release-linux-228 configure/build/test completed all 472 build steps and passed 13/13 CTest smoke targets. Remote PR checks are also green across all product builds/smoke tests/docs regeneration. Focused runtime probes on that build nevertheless returned "adXc" for an ordered concat whose original result is "aXcd", rewrote (/ a 49) to (/ a 49.00000000000001), and returned 0.9999999999999999 for the corresponding a=49 call. The zero-arity sorted-unparse case happens to print successfully in this release build, but constructs an invalid random-access iterator range and is undefined behavior.
I inventoried all six historical threads before reviewing current HEAD. The null-result crash, boolean absorbing value, distinguished subtraction/division operand/arity, and generic Merger<T> issues are fixed and are not restated. The prior division thread is only partially superseded because the reciprocal round-trip below remains. Two independent review lanes (Claude Code primary, Codex second-opinion) agreed on the ordered-concat and invalid-sort-range blockers; both also identified the reciprocal canonicalization defect. No other blocking findings remain from this pass.
howso-mario
left a comment
There was a problem hiding this comment.
CHANGES_REQUESTED at 6474cece. CI-faithful verification passed: ghcr.io/howsoai/amalgam-build-container-linux-228:2.0.9, preset amd64-release-linux-228, configure + full 472-target build + CTest 13/13. Live GitHub checks are 19 successful and 5 expected release-only skips. Focused probes on ST, ST-PGC, ST-AFMI, MT, and MT-AFMI reproduced the semantic differences in the inline blockers.
howso-mario
left a comment
There was a problem hiding this comment.
Re-reviewed current head 39a12b1 against the previously verified bd5e2ae head. The four findings from review 4912005498 remain resolved: zero-divisor ordering is preserved, unsafe exp/log inverses remain removed, symbolic subtraction/division heads are not cancelled, and node-type mutation paths return before reusing invalidated child vectors. The post-review delta is limited to comment cleanup, a clean merge of main’s C++20/build update, and consistent Visual C++ stdcpp20 settings. All current required CI checks, including Windows, GCC 14 Linux/glibc 2.28, ARM, macOS, WASM, docs, smoke tests, and final-check, are green; release-only jobs are skipped as expected.
Uh oh!
There was an error while loading. Please reload this page.