[lua] Port _hx_bit_clamp to haxe code#12600
[lua] Port _hx_bit_clamp to haxe code#12600tobil4sk wants to merge 12 commits intoHaxeFoundation:developmentfrom
Conversation
We can now rely on normal dce so that the code for this function isn't generated unless required. It also provides more control over what code is generated. This also allows compile time abstractions that aren't supported on lua, like inline function calls, which reduce duplicate code.
This is a simpler operation which makes more sense here given the range of values this.length can have. Array fails to be dce'd in hello world, so removing this avoids clamp from being included.
In clampInt32, these are used before static init runs, which means they are set to nil
If lua 5.3+ is being targetted, we know that native bitwise operators are available, so we can use them. On lua 5.2, we know that bit32 is available so we don't need to generate a fallback for it being null.
|
|
||
| // Ideally, these should be extern inline because they should only be used | ||
| // via inlining (we also don't want clampNativeOperator outside testFunctionSupport) | ||
| // however, extern inline prevents closures, even if they are immediately inlined |
There was a problem hiding this comment.
This is something that needs to be addressed, otherwise using --dce=no generates clampNativeOperator code outside testFunctionSupport which breaks older versions.
There was a problem hiding this comment.
@Simn Would you have any suggestions here? I tried turning clampWrapper into a macro, but that gives:
[ERROR] /.../haxe/std/lua/Boot.hx:283: characters 48-81
283 | final nativeOperators = testFunctionSupport(clampWrapper(clampNativeOperator));
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
| You cannot access the lua package while in a macro (for lua.Boot)
Even if everything in that file is wrapped with #if !macro
|
I looked into why Code generation time (after DCE): DCE time: Fix: Remove
If nothing calls Also worth noting: |
Previously use._bitop was being set during the generator phase, at which point dce already occurred so @:ifFeature on lua.Boot.clampInt32 didn't work. This patch moves use._bitop detection to the dce phase, which means it will be set in time to make sure clampInt32 is included when needed.
-Min_Int32 is "optimised" into -2147483648 due to overflow
This analysis seems correct, but claude was a bit too confident about the solution. I did some digging and it seems like python already deals with the same problem: #11427. I lifted the solution from there, and the test seems to pass now. |
The clamp port caused a regression in underflow, but it wasn't caught due to this missing test case.
Haxe modulo generates as math.fmod, which rounds towards 0 instead of minus infinity. This breaks underflow. This requires adding lua.Syntax.modulo. Documentation is taken from: https://www.lua.org/manual/5.5/manual.html#3.4.1
We can now rely on normal dce so that the code for this function isn't generated unless required. It also provides more control over what code is generated, which has allowed adding checks like lua_ver >= 5.3 to avoid redundant compatibility implementations.
This also allows compile time abstractions that aren't supported on lua, like inline function calls, which reduce duplicate code.
The generic implementation checks what is supported to decide which implementation to use, then patches the method to avoid having to rerun the feature detection every time. This can't go into static init because bitwise operations are needed in regex constructors, which are called in some static initialisers. I think this way is also fairly self contained which is nice.
Currently a test is failing because
__lua_Boot.clampInt32is nil, I think my check@:ifFeature("use._bitop")isn't sufficient.Below is the generated lua code, which could be improved, but I'm surprised at how close it is to the original!
Expand to view
Related to #8852.