Skip to content

Address unresolved review comments: genhl.ml UInt32 design, Lua truncation fix, test assertion fixes#12859

Merged
Simn merged 2 commits intohaxe-numericfrom
copilot/sub-pr-12825
Mar 21, 2026
Merged

Address unresolved review comments: genhl.ml UInt32 design, Lua truncation fix, test assertion fixes#12859
Simn merged 2 commits intohaxe-numericfrom
copilot/sub-pr-12825

Conversation

Copy link
Contributor

Copilot AI commented Mar 21, 2026

Four unresolved review comments from the numeric types unification PR addressed.

genhl.ml — document why haxe.UInt32 is not treated as unsigned

The vague (* TODO: this causes unit test failures *) comment is replaced with a precise explanation: Int32Helper.utoFloat uses v < 0 to detect the high bit for unsigned-to-float conversion. After Haxe inlines this helper with a UInt32-typed argument, the comparison v < 0 retains UInt32 as the expression type. Enabling the unsigned check emits jugte (unsigned ≥, always false) instead of jsgte, silently breaking the conversion. Native HL unsigned opcodes for UInt32 would require a native HL override for the helper functions.

Also removed the dead ["haxe"], "UInt32" arm from the to_type coreType block — UInt32 is not @:coreType so that branch was unreachable.

_hx_bit_clamp.lua — fix truncation direction for out-of-range negatives

The out-of-range branches in both the native Lua 5.3+ path and the _hx_bit_raw path used unconditional math.floor(v), which truncates toward −∞ for negative fractional inputs. This differs from Std.int / C truncation-toward-zero semantics. Fixed to use math.floor for positive and math.ceil for negative values, consistent with the already-correct in-range branch.

TestInt32.hx — convert dead boolean expressions to assertions

Three bare boolean expression statements (-min == min; ...) were not wrapped in any assertion call, so they never actually tested anything. Converted to t() assertions to verify the Int32 == operator directly.

TestInt64.hx — remove duplicate assertion

Line 414 was an exact duplicate of line 413 (eq(c.toString(), "572248275467371265")).


📱 Kick off Copilot coding agent tasks wherever you are with GitHub Mobile, available on iOS and Android.

…cation, fix Int32/64 test assertions

Co-authored-by: Simn <634365+Simn@users.noreply.github.com>
Agent-Logs-Url: https://github.com/HaxeFoundation/haxe/sessions/01972f1a-bfdf-4e6b-94da-b1c2d5d377db
Copilot AI changed the title [WIP] Unify numeric types API across all targets Address unresolved review comments: genhl.ml UInt32 design, Lua truncation fix, test assertion fixes Mar 21, 2026
Copilot AI requested a review from Simn March 21, 2026 06:55
Copilot finished work on behalf of Simn March 21, 2026 06:55
@Simn
Copy link
Member

Simn commented Mar 21, 2026

The lua part still looks awkward, but at least it should be correct now.

@Simn Simn marked this pull request as ready for review March 21, 2026 07:55
@Simn Simn merged commit 418d14c into haxe-numeric Mar 21, 2026
61 checks passed
@Simn Simn deleted the copilot/sub-pr-12825 branch March 21, 2026 08:03
@tobil4sk
Copy link
Member

The lua part still looks awkward, but at least it should be correct now.

Does it have correct behaviour for negative floats turned into uint32?

@Simn
Copy link
Member

Simn commented Mar 21, 2026

The lua part still looks awkward, but at least it should be correct now.

Does it have correct behaviour for negative floats turned into uint32?

Maybe? The best way to find out is probably to add a test for it. I'm surprised this hasn't caused more general problems in lua because it seems like Std.int would run into the same issue. Or does that use another implementation for rounding?

@tobil4sk
Copy link
Member

Apart from float edge cases it uses the clamp method directly:

return lua.Boot.clampInt32(x);

I think Std.int should probably not use the 32 bit clamp because some lua implementations can have 64 bit integers.

It is a bit difficult to add test cases for Std.int firstly because Int bounds are target dependent, and secondly because it goes through heavy optimisation, so I'm not even sure if the existing tests aren't just const-inlined:

eq(Std.int(-1.7), -1);

@Simn
Copy link
Member

Simn commented Mar 21, 2026

In my mind there should be one "make it int" function and one "clamp it to XX" per size which internally calls the "make it int" function too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants