fix(lua): compare float with int strictly in equality (#7587) - #7611
fix(lua): compare float with int strictly in equality (#7587)#7611bultodepapas wants to merge 3 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes Lua numeric equality in the embedded Lua 5.3 VM so that float–integer comparisons via == are strict (only integral floats can equal integers), preventing cases like 0.5 == 0 incorrectly evaluating to true when LUA_FLOORN2I is enabled for API argument coercion.
Changes:
- Added a strict integer-conversion macro (
tointegerns) that only converts integral floats to integers. - Updated
luaV_equalobjto use the strict conversion for float–integer equality. - Added a regression test covering non-integral and integral float–integer equality behavior.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| radio/src/thirdparty/Lua/src/lvm.h | Adds tointegerns macro for non-soft (strict) float→int conversion. |
| radio/src/thirdparty/Lua/src/lvm.c | Switches mixed-variant numeric equality to strict integer conversion. |
| radio/src/tests/lua.cpp | Adds a regression test ensuring 0.5 ~= 0 while 1.0 == 1 still holds. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Thanks for throwing your hat in. Can you add tests for <= and >= with the same test values? I didn't try those in my original testing. |
|
Thanks for the review! I've added <= and >= tests for the same values in radio/src/tests/lua.cpp (commit 19dc95d):
Order comparisons (<, <=, >, >=) compare the float and integer directly as numbers (via luai_numlt/luai_numle), so they were already correct - only ==/~= went through the integer-conversion path. I verified all of the above by compiling the Lua runtime standalone. |
Extend the EdgeTX#7587 regression test to cover: - the reported scenario via a variable (v = 0.50) - negative non-integral floats (-0.5) - all comparison operators (==, ~=, <, >, <=, >=) - mixed arithmetic still yielding floats (math.type) Also document in lvm.h why equality uses strict (non-soft) conversion while EdgeTX API argument coercion keeps the soft LUA_FLOORN2I behavior.
|
While extending the regression test, I checked the remaining Lua 5.3 surface for the same root cause (
math.tointeger(3.7) --> 3 -- stock Lua 5.3: nil
math.tointeger(0.5) --> 0 -- stock Lua 5.3: nilI deliberately did not change it here. The EdgeTX API layer relies on soft argument coercion, and changing |
|
Tests look good. I'd prefer leaving math.tointeger alone for now, as that is out of scope of the original issue. Changing that might affect existing LUA scripts that have been written recently and expect the "floor" behavior. Making that change will delay acceptance of the PR, since the devs would have to verify it doesn't break existing scripts. In contrast, your current fix would restore pre-existing behavior, which is what we want. Ah, this didn't get the labels from the issue because you made it a standalone PR. I suggest you add the "bug", "triage", and "lua" labels in order for this PR to have any visibility. |
|
Heads-up for maintainers: per the review feedback, this PR still needs the The requested |
|
@pfeerick, when you have time, could you take a look at this Lua VM regression fix? The requested comparison coverage is in |
Fixes #7587
Summary
0.50 == 0(and0.5 == 0) incorrectly returnedtruein the EdgeTX Lua runtime. This breaks floating point comparisons in Lua scripts.Root cause
LUA_FLOORN2Iis set to1inluaconf.hso EdgeTX API functions can accept unrounded floats where integers are expected. However, the same soft conversion was also used byluaV_equalobjfor the==operator: comparing a float and an integer converted the float withluaV_tointeger(..., LUA_FLOORN2I)(i.e. flooring), so0.5was turned into0and0.5 == 0becametrue.Stock Lua 5.3 uses the strict (
tointegerns) conversion for equality: only integral floats compare equal to integers.Fix
tointegernsmacro inlvm.h(same as stock Lua 5.3): converts floats to integers only when the value is integral.tointegernsinluaV_equalobjinstead oftointeger.LUA_FLOORN2Ibehaviour for API arguments is unchanged.Tests
Added
Lua.testFloatIntegerEqualityinradio/src/tests/lua.cppverifying:0.5 ~= 0and0.50 ~= 0are nowtrue1.0 == 1,0.0 == 0still holdVerified by compiling the Lua runtime standalone and running the assertions.