Repository navigation
feat: add bit32.to_signed and speed up the byte helpers and pure Lua backend - #21
Conversation
The inverse of to_unsigned: values of 2^31 and above wrap to negative, and values already in signed range pass through.
![svc-finitelabs[bot]](https://avatars.githubusercontent.com/in/3141848?s=60&v=4){kind=small}
There was a problem hiding this comment.
Reviewed at head 7102500. Verdict: correct, and the tests are real. The formal approve is withheld for the auto-merge reason at the bottom, not because of any doubt about the change.
Correctness. to_signed is an exact two-sided inverse of to_unsigned over the intended domains. to_unsigned is n < 0 -> n + 2^32, so for x in [-2^31, 2^31-1] the negatives land in [2^31, 2^32-1] and come back unchanged, and the non-negatives pass through in both directions. Same result going the other way for u in [0, 2^32-1].
The negative passthrough is load-bearing, not laxness. zigzag_decode32 builds its input from raw_bxor/raw_rshift, which on LuaJIT return signed 32-bit values, so this function has to accept an already-signed input and leave it alone. Anyone later tempted to harden it by masking the input would break the exact call site it was written for. The to_signed(-1) vector is what pins that behaviour, so it earns its place even though it looks redundant next to to_signed(0xFFFFFFFF).
The tests discriminate. I replaced the body with return n and re-ran: to_signed(0x80000000), to_signed(0x80000001) and to_signed(0xFFFFFFFF) fail on both interpreters I have. The vectors are not passing vacuously.
Ran a version the matrix does not cover. Local lua here is 5.5.0, which is not in the CI matrix, plus LuaJIT 2.1. 105/105 and 114/114, with all seven new vectors passing on both. Together with CI green on 5.1, 5.2, 5.3, 5.4, luajit-2.0 and luajit-2.1, that is a wider number-model spread than the PR body claims. make typecheck is clean locally under luals 3.19.0, the same version CI pins. (make lint will not run on my box: luacheck 1.2.0 assigns to a const in standards.lua, which 5.5 rejects. My environment, not the repo.)
Checked the downstream claim. src/protobuf/init.lua:485-487 in lua-protobuf is byte-identical logic, and finitelabs/lua-protobuf#28 adds a second copy of the same three lines for the low word. Both are clean swaps once this is released and re-vendored. The FL-18 sfixed32 site does not exist yet, so that third one is anticipated rather than verified. Worth flagging that the re-vendor is downstream of #28, which is currently CONFLICTING and blocked on #27.
One nit, failure path only. These are the first vectors in the main loop with a negative expected, and the printer is string.format(" Expected: 0x%08X", ...). Under a failure that renders as 0xFFFFFFFF80000000 rather than 0x80000000, on both 5.5 and LuaJIT. Cosmetic, and only visible when something is already broken, so entirely your call whether it is worth a %s or a mask.
Not approving formally: this repo is where auto-merge was armed mid-review on #14, and with all eight checks green an approve from me would be the last gate and could merge to main itself. A comment review puts the verdict on record and leaves you to pull the trigger. Say the word and I will convert it to an approve.
string.pack/unpack where available, a single 8-byte string for the 64-bit encoders, and % instead of the per-bit pure Lua band. Also prints negative expected values in the bit32 selftest as decimal.
derek-miller
changed the title
|
Pushed a second commit on top of |
There was a problem hiding this comment.
Reviewed at head 02e6c4c, CI 8/8 green. Approving.
What I did differently this round. Rather than re-read the arithmetic, I built an old-vs-new differential: loaded 7102500 and 02e6c4c side by side in one process and compared outputs over the u32 and u64 encoders in both byte orders, the decoders across every valid offset in a 64-byte blob, and from_number across ±2^53 plus the boundaries. 26,539 comparisons, zero differences. Separately, every 8-byte encode round-trips back to the wrapped input.
The part that makes that number mean something: all three implementation arms are covered, and each is proven live. This commit adds a string.pack path, keeps the compat_band path, and adds a new %-masking path for pure Lua. A differential that only ever ran one of them would report a confident zero. So I planted a mutation in each arm and confirmed it lights up in that arm only:
| planted bug | Lua 5.5 | LuaJIT 2.1 | conclusion |
|---|---|---|---|
<I4 to >I4 in u32_to_le_bytes |
3,023 diffs | 0 | 5.5 is on the pack path, LuaJIT is not |
two bytes swapped in the u64_to_be_bytes fallback |
0 | 5,990 diffs | LuaJIT is on the fallback |
+ 1 on the n % 0x100000000 arm |
0 | 3,026 diffs (bit lib hidden) | the pure Lua arm is reached |
To get that third arm I forced _compat down "Implementation 3", running LuaJIT so the native-operator probe fails on 5.1 syntax, with bit and bit32 hidden. That is the configuration CI's plain 5.1 entry resolves to, since the test job installs no luarocks and 5.1 has no built-in bit32, so the new % arm really is exercised on CI rather than only by me. Selftests pass in that mode too: 74/74, 105/105, 116/116.
The two shortcuts are safe for the reason they look unsafe. setmetatable({high, low}, Int64Meta) skips bit64.new, whose only work is or 0 defaulting and a single + 2^32 negative normalisation. In the decoders the words come from >I4/<I4, so they are already in [0, 2^32) and neither branch could fire. In from_number the new % 0x100000000 on the high word subsumes that normalisation and extends it past one wrap, which is exactly the -2^70 case you documented. The inline type(value) == "table" and getmetatable(value) == Int64Meta is character-for-character what is_int64 does at bit64.lua:68.
fast_band and compat_band(n, MASK32) or n % 0x100000000 is the one line where that idiom would usually be a bug, and it is correct here only because 0 is truthy in Lua, so a band result of 0 does not fall through to the right arm. Worth knowing it is load-bearing.
Three notes, none blocking.
1. One out-of-domain input goes from a loud error to a silent wrong answer, on some interpreters only. be_bytes_to_u32(str, 0) used to raise everywhere, because string.byte(str, 0, 3) yields three values and b4 is nil. On the string.pack path it now returns the offset-1 answer:
be_bytes_to_u32("\1\2\3\4...", 0) old 5.5: error new 5.5: 16909060
old JIT: error new JIT: error
assert(#str >= offset + 3) does not reject offset < 1, and string.unpack treats position 0 as 1. Offset is documented 1-based so this is out of contract, but it is reachable from a caller doing zero-based position tracking, which is what a protobuf decoder does. What I would weigh is not the wrong answer itself, it is that the answer now depends on the interpreter: loud on 5.1, 5.2 and LuaJIT, quiet on 5.3 and 5.4. Extending the existing assert with offset >= 1 closes it.
2. The body's list of changed out-of-range outputs is one short. from_number(1/0) was {inf, nan} and is now {nan, nan}, the high word having picked up the %. Both are garbage and I would not touch the code, but the body is precise about which inputs moved, so it may as well be complete.
3. The two deliberate semantic changes ship unpinned. Vector counts are identical across the heads, 105/105 and 116/116, so this commit adds none. The existing vectors do cover all three arms, per the table above, so the paths are not untested. But nothing asserts from_number(-2^70) == {0, 0} or that -0.0 yields a 0 high word. Those are the two behaviours you changed on purpose, and they are the two a later refactor could revert without turning anything red.
Ran locally: make typecheck clean under luals 3.19.0, the version CI pins, make format-check exit 0, working tree clean afterwards. make lint and make build will not run on my box (luacheck 1.2.0 assigns to a const in standards.lua, which 5.5 rejects, and amalg is not installed); both are green in CI. The differential ran on Lua 5.5 and LuaJIT 2.1, and 5.5 is not in your matrix, so that is one number model beyond what CI checked.
Auto-merge is not armed, so this approve is a verdict and the merge is still yours to press.
…'s wrap string.unpack treats offset 0 as 1, so offset 0 silently read the wrong bytes on 5.3+ while raising elsewhere.
|
Pushed the three notes from your approval: offsets below 1 now raise in all four byte decoders, new vectors pin |
There was a problem hiding this comment.
Re-approving at 89c57e9. All three notes from the 02e6c4c approval are addressed, and I mutation-tested each new vector rather than reading the green run as evidence they work.
Offset guard. assert(offset >= 1, ...) sits ahead of the length assert in all four decoders that gained a string.unpack fast path, so it is what fires, not the length check. Deleting just those four lines turns the new tests red on the pack path (Lua 5.5 here: bit32 105/107, bit64 119/121) and leaves them green on LuaJIT, where the string_byte fallback already raised on the nil fourth byte. That is the right shape: the regression test discriminates on exactly the runtimes that had the silent wrong answer, and the matrix carries both arms.
from_number vectors. Dropping the % 0x100000000 from the high word turns from_number(-1) and from_number(-2^70) red on both interpreters, so neither vector is passing for an incidental reason.
The -0.0 probe. 1 / from_number(-1/math.huge)[1] > 0 cannot fail on 5.3+, because math.floor returns an integer there and an integer has no signed zero. It does discriminate on 5.1, 5.2 and LuaJIT, which is where a -0 high word is representable at all, and I confirmed a planted -0.0 high word is caught there. Vacuous on half the matrix by construction rather than by accident, so nothing to change.
Description. from_number(1/0) is now listed, and "the first three are pinned by new selftest vectors" matches what landed.
One non-blocking note, for whenever bit16 is next touched. bit16.be_bytes_to_u16 and bit16.le_bytes_to_u16 also take an offset and did not get the guard, so "the four byte decoders" is four of six. They are safe today, but for a reason that is not the guard: bit16 has no string.unpack path, so string_byte(str, 0, 1) yields a single value and the arithmetic on the nil second byte raises identically on every runtime. The divergence this PR closes does not exist there. It would come back the moment bit16 picks up the same fast path, and in the meantime the error text is inconsistent ("attempt to perform arithmetic on a nil value" against "Offset must be at least 1"). Not worth another round now.
Verified at this head: all 8 checks green, make typecheck clean, make format-check exit 0, selftests 107/107 and 121/121 on Lua 5.5, 116/116 and 121/121 on LuaJIT 2.1, tree clean after each mutation was reverted. I did not run make lint or make build locally: luacheck 1.2.0 hits a const-assign error under 5.5 on this box and amalg is not installed. Both are green in CI.
…s offsets The per-call assert made the decoders slower than v0.6.2. error() keeps the file:line prefix assert produced.
|
One more push before merge, sorry to dismiss your approval again. Benchmarking each version in its own process showed the approved head decoding slower than v0.6.2: the per-call |
|
Please hold the review on this head: one more commit is coming. It speeds up bitn's pure-Lua backend ( |
The per-bit loops made every bitwise op up to 32 iterations with two floor calls each on runtimes without native or library bit ops.
derek-miller
changed the title
|
Pushed the pure-Lua backend commit: |
There was a problem hiding this comment.
Re-reviewed the whole PR at 4e9c3d3. Base unchanged at a20cafe, 8/8 checks green, make typecheck clean, make format-check exit 0. Approving.
Two passes here: the new pure-Lua backend commit, and the three notes from the ab64bc9 pass I had finished when you asked me to hold. I re-ran those two code notes against 4e9c3d3 rather than assuming they carried, and both reproduce verbatim.
4e9c3d3, the pure-Lua backend
Correctness: independently confirmed, by a different method than yours
Your equivalence run is old-backend against new-backend. Re-running that would only echo it, so I checked against a different reference: LuaJIT's native bit library, with bitn forced down the pure arm in the same process, so the oracle and the subject see identical inputs.
175,692 pairs across band, bor and bxor, zero mismatches. Inputs were every 2^k - 1, 2^k and 2^k + 1 for k in 0..32, the usual patterns (0xAAAAAAAA, 0x55555555, 0xDEADBEEF, 0xFFFFFFFF), negatives (-1, -2, -255, -0x80000000), and 120 deterministic pseudo-random words.
I validated that harness rather than trusting a zero: planting LOW_MASK[2^k - 1] = 2^(k+1) turns it into 4,776 mismatches. A zero from an instrument that cannot go non-zero is not a result.
Reading the code alongside that: while a > 0 and b > 0 is right for band and while a > 0 or b > 0 is right for bor/bxor, the a == 0 / b == 0 early returns are placed after the % 0x100000000 normalisation so they return normalised values, and (LOW_MASK[b] and a or b) is safe only because 0 is truthy in Lua, same load-bearing idiom as the fast_band and ... or ... line above it.
The mask fast path has 26 of its 32 input classes untested
This is the one worth acting on. The commit adds no selftest vectors, so the new code is covered only incidentally by whatever the existing suite happens to pass through it.
I instrumented _compat.band to record which low-mask widths actually reach the fast path during a full bit16 + bit32 + bit64 selftest run:
mask widths EXERCISED : 1(1x) 2(1x) 8(1x) 16(105x) 31(2x) 32(288x)
mask widths NEVER hit : 3 4 5 6 7 9 10 11 12 13 14 15 17 18 19 20 21 22 23 24 25 26 27 28 29 30
Six of 32, and widths 16 and 32 are essentially all the traffic. To show that is a real gap and not a theoretical one, I made band(x, 0x1F) return the constant 12345:
| result | |
|---|---|
| my oracle harness | 483 mismatches |
| selftests, pure arm | 76/76, 107/107, 121/121, all green |
| selftests, native LuaJIT | 76/76, 116/116, 121/121, all green |
A five-bit mask returning a wholly wrong constant ships green through the entire suite on both arms. Note also that the row you benchmark as the headline win for this path, raw_band(a, 0x7F) at 2030 → 101, is width 7, which is on the never-hit list. The fastest new path is the untested one.
Breaking all the masks at once is caught, but only through bit16 and bit64; bit32 stays at 107/107, so the module that most directly wraps _compat.band is not what covers it. That makes the current coverage both partial and accidental, and a future refactor of bit16's internals could remove it without anything going red.
A loop over k in 1..32 asserting band(0xDEADBEEF, 2^k - 1) == 0xDEADBEEF % 2^k is about six lines and closes it completely.
Minor: the fast path returns a float on 5.3+, the slow path an integer
LOW_MASK[2 ^ k - 1] = 2 ^ k stores floats, because ^ always produces a float on 5.3+, so a % m is a float too. Forced onto the pure backend on 5.5:
band(0xFF00FF, 0xFF) [fast path] = 255.0 math.type = float
band(0xF0F0F0, 0xAAAA) [slow path] = 41120 math.type = integer
No production impact, since 5.3+ takes the native-operator arm and 5.1/5.2 have no integer subtype. It only shows up in the forced configuration you verify with, which means those 5.3/5.4 runs exercise a slightly different shape than 5.1 actually gets, and tostring gives 255.0 where the rest of the library gives 255. Building the table by doubling keeps it integral everywhere:
local LOW_MASK = {}
do
local m = 1
for _ = 1, 32 do
m = m * 2
LOW_MASK[m - 1] = m
end
endVerified: fast path becomes integer 255 on 5.5, the oracle run stays at zero mismatches on LuaJIT, and selftests stay 76/76, 107/107, 121/121.
Your benchmark table for this commit is fine
Flagging this because my note below says the opposite about the decoder table, and I do not want it read as a blanket objection. This commit's numbers run 49 to 4202 ns/op, far above the harness floor, so they are measuring real work. The decoder rows are the problem, not these.
Carried from the ab64bc9 pass, re-verified at 4e9c3d3
1. assert(x >= y) to if x < y inverts under a NaN offset
assert(offset >= 1) fires on NaN because NaN >= 1 is false. if offset < 1 does not, because NaN < 1 is false too. Same inversion on all six length guards, so a NaN offset now falls through both checks into the decode path:
le_bytes_to_u32(s, 0/0) |
Lua 5.5 | LuaJIT 2.1 |
|---|---|---|
89c57e9 |
Offset must be at least 1 |
Offset must be at least 1 |
4e9c3d3 |
bad argument #3 to 'string_unpack' (number has no integer representation) |
attempt to perform arithmetic on local 'b2' (a nil value) |
bit16 moves the same way, from a uniform Insufficient bytes for u16 to bad argument #2 to 'string_byte' on 5.5 against nil arithmetic on LuaJIT.
Out of contract, since @param offset? integer, and it still raises either way, so not a blocker. I raise it because it is the same shape this series has spent two rounds closing: a uniform contract error becoming an interpreter-specific internal one, in a library whose job is cross-interpreter uniformity.
Keeping the positive predicate fixes it without giving back the speed:
if not (offset >= 1) then
error("Offset must be at least 1")
end
if not (#str >= offset + 3) then
error("Insufficient bytes for u32")
endAcross all 12 guards: NaN returns to a uniform Offset must be at least 1 on both interpreters, selftests stay green, and le_bytes_to_u32 on Lua 5.5 measures 82.8/85.3/82.4 ns with assert, 53.0/53.3/56.3 with if offset < 1, and 53.6/57.2/57.1 with not (offset >= 1).
2. The two new bit16 vectors pass whether or not the guard exists
Deleting both bit16 offset guards leaves 76/76 on Lua 5.5 and 76/76 on LuaJIT. The same mutation is caught elsewhere: bit32 drops to 105/107 and bit64 to 119/121 on 5.5.
Cause is what I flagged last round: bit16 has no string_unpack path, so string_byte(str, 0, 1) already returned one value and the nil arithmetic already raised everywhere. What the guard adds for bit16 is the message, and if not pcall(test.fn, "\1\2\3", 0) then only asserts that something was raised. Worth keeping as insurance for the day bit16 gains the fast path, but as written the vector cannot detect its own guard's removal on any runtime.
The suite already has the stronger idiom, at bit64.lua:1193 and bit64.lua:1223:
if not ok and type(err) == "string" and string.find(err, "Offset must be at least 1") thenThat version discriminates on all six decoders instead of two.
3. The LuaJIT column of the decoder table is measuring the empty loop
benchmark_op discards the result and the inputs are loop-invariant, so LuaJIT optimises the call away. Through the repo's own harness on this box:
| LuaJIT 2.1, 100k iterations | ns/op |
|---|---|
| empty function (floor) | 1.16 |
le_bytes_to_u32 |
0.89 |
le_bytes_to_u64 |
1.02 |
The decoders measure faster than doing nothing. So le_bytes_to_u32 at 1.9 → 1.9, and the earlier comment's "the assert cost LuaJIT le_bytes_to_u64 about 35%", are differences between two noise readings. I could not reproduce the 35%: three reps at each head give no consistent direction.
I am not asserting your run was wrong. Your le_bytes_to_u64 figure is 27.2 ns against my 1.02, a 27x gap, so your luaenv build may not be sinking the allocation the way mine does, in which case your number is the real one. The control is a ten-second check: add benchmark_op("EMPTY", function() end, iterations) and see where the floor lands.
The Lua side reproduces strongly and justifies that commit on its own. Repo harness, Lua 5.5, three reps:
89c57e9 |
ab64bc9 |
|
|---|---|---|
be_bytes_to_u32 |
79.7 / 81.9 / 81.5 | 58.5 / 56.6 / 55.3 |
le_bytes_to_u32 |
83.3 / 83.4 / 83.5 | 56.3 / 55.3 / 56.2 |
be_bytes_to_u64 |
152.5 / 156.0 / 153.2 | 126.1 / 128.7 / 129.0 |
le_bytes_to_u64 |
152.8 / 155.1 / 150.8 | 126.8 / 129.2 / 128.6 |
Roughly 32% off the u32 decoders and 16% off the u64 ones, consistent across reps, same direction and size as your 5.1 figure.
Adjacent: benchmark_op prints %8.3f ms/op, which is 0.000 for every byte helper, so none of the ns/op numbers in the description can be reproduced from make bench as shipped without back-computing them from the ops/sec column.
Checked and clean
- The
file:line:claim holds, and I tested it rather than taking it. I expected the opposite, thatassertraises bare anderror()adds position, which would have made this a message-format break.assertroutes througherrorat level 1, so both produce the prefix; a differential over all six decoders on both interpreters gives identical guard messages modulo the line number. Level 1 is also the right choice over level 2, which loses the prefix entirely when the caller tail-calls the decoder, confirmed on both interpreters. - Guard order is right: the offset check precedes the length check, so it is the one that fires, and the offset-0 test strings still clear the length check.
- In-range decoder output unchanged: full differential across all six decoders at both heads on both interpreters.
- Selftests at
4e9c3d3on Lua 5.5, LuaJIT, and LuaJIT forced onto the pure arm: 76/76, 107/107, 121/121 and 76/76, 116/116, 121/121. - The description was updated correctly to "all six byte decoders", and the
bit16guards match thebit32andbit64text exactly.
Pre-existing, not introduced here
No Insufficient bytes vector exists anywhere. Deleting all six length guards leaves 76/76, 107/107 and 121/121 on both interpreters. Same at a20cafe, so this PR did not cause it, but it does mean the rewrite of those six lines landed with no test able to see a mistake in the rewritten predicate, and note 1 is exactly such a mistake with nothing going red.
Summary
Approving. Correctness is confirmed independently on both commits, and CI is green across all eight entries.
Ranked by what I would want before merge: the mask-coverage gap first, since it is new code where a demonstrated wrong answer ships green and the fix is a six-line loop; then note 1, a one-token change per guard at measured zero cost; the float/integer and harness items are cleanup whenever.
Not merging, and my approval is not Derek's.
Did not run make lint or make build locally (luacheck 1.2.0 hits a const-assign under Lua 5.5, amalg is not installed here); both green in CI.
The offset guard checks for NaN explicitly, so a NaN offset raises the contract error on every interpreter. Vectors check the message and cover short buffers, NaN offsets and every low-bit mask width. LOW_MASK is built by doubling so 5.3+ gets integers.
|
Pushed the notes from your approval.
On the LuaJIT decoder rows: my harness feeds every result into an accumulator ( |
There was a problem hiding this comment.
Re-reviewed at bf25cbc, scoped from my last reviewed head 4e9c3d3 (my approval there was auto-dismissed by the push). All four notes are addressed. I verified each by mutation rather than by reading.
Where the backends actually split. The pure Lua backend is selected on exactly one CI entry, Lua 5.1: 5.2 has a built-in bit32 so it takes the bit-lib arm, 5.3/5.4 take native operators, and both LuaJIT entries take bit. Separately the decoders branch on string.unpack, which is a different axis: absent on 5.1, 5.2 and both LuaJITs. So I modelled the four real configurations on 5.5 by defeating the & probe and/or clearing string.pack/string.unpack before load, and ran every mutation against all four. Baseline is green on all of them: 317/317 on the 5.1 and 5.4 models, 326/326 on the 5.2 and LuaJIT models.
The mask-width test has teeth, and they are on Lua 5.1 only. Last round band(x, 0x1F) returning a constant passed every module. Re-planted against the new suite:
| mutation | 5.1 model (pure) | 5.2 / LuaJIT model | 5.3 / 5.4 model |
|---|---|---|---|
band(x, 0x1F) returns a constant |
1 fail | 0 | 0 |
LOW_MASK values doubled |
24 fail | 0 | 0 |
OR4 nibble table corrupted |
31 fail | 0 | 0 |
XOR4 nibble table corrupted |
10 fail | 0 | 0 |
The zeroes on the right are correct rather than a gap, since LOW_MASK and the nibble tables only exist on the pure backend. The consequence worth writing down: Lua 5.1 is load-bearing for the entire pure-backend perf change in ab64bc9 and 4e9c3d3. Dropping it from the matrix would silently retire coverage of all of it.
The offset guards, with one correction to your note. "Removing the offset guards fails four vectors per module" holds for bit16 and bit32 on every interpreter, but not for bit64:
| guards removed | 5.1 model | 5.2 / LuaJIT model | 5.3 / 5.4 model |
|---|---|---|---|
| bit16 | 4 | 4 | 4 |
| bit32 (NaN half only) | 2 | 2 | 2 |
| bit64 | 0 | 0 | 4 |
Not a defect. Without string.unpack, bit64.*_bytes_to_u64 falls through to bit32.*_bytes_to_u32, whose guard raises the identical Offset must be at least 1, so the contract holds on every interpreter and the vectors still pass. They just are not isolating bit64's own guard there. It is load-bearing only on 5.3/5.4, where the string_unpack branch returns before any bit32 call. Flagging it so the vector count is not read as proof that the bit64 guard is exercised everywhere.
The fix itself is right and uniform. NaN, -inf and 0.5 all raise Offset must be at least 1 now, on both interpreters I have. +inf raises Insufficient bytes, which is the correct answer rather than an offset error. Short-buffer vectors discriminate as well: removing bit32's #str guard fails one vector on all four configs. Checking the message with string.find is a genuine strengthening, not cosmetic. It is what makes the NaN vectors fail loudly instead of passing on whatever internal error the decode path happened to throw.
LOW_MASK integer construction. Confirmed it does what it says: the fast path returns 111 where the old 2 ^ k form returned 111.0. Worth noting it is unpinned, and as far as I can tell necessarily so. Reverting to the float form still passes 317/317, and no vector can catch it in CI, because integer and float subtypes only exist on 5.3+ while the pure backend only runs on 5.1. Raising it so the asymmetry reads as intentional rather than as a vector you missed.
The bit32.lua comment now matches the code.
What I ran. Selftests on Lua 5.5 (native) and LuaJIT 2.1 (bit), plus 5.5 with the backend and/or string.pack/string.unpack forced off to model 5.1 and 5.2. Nine mutations, each reverted with a clean-tree assert, in a scratch copy. make typecheck clean, make format-check clean. I did not run make lint or make build locally: luacheck 1.2.0 fails to load under Lua 5.5 on this box and amalg is absent, so I am relying on CI's Check and Build jobs for those. All 8 checks are green at bf25cbc.
Approving. Not merging: a bot approval is not yours, and I have not seen you approve this one.
1 participant
Two changes, both for v0.6.3.
bit32.to_signed, the inverse ofbit32.to_unsigned: values of 2^31 and above wrap to negative, and values already in signed range pass through. finitelabs/lua-protobuf hand-rolls this conversion in three places.Faster byte helpers and
from_number. lua-protobuf is moving its fixed32, fixed64, float and double codecs onto these helpers, and they were slower than the inline arithmetic they replace.string.pack/string.unpackwhere the runtime has themu64_to_le_bytes/u64_to_be_bytes, instead of two 4-byte strings and a concat%for masking when there is no native or bit-libraryband, since the pure Luabandis a per-bit loopfrom_numberchecks the metatable inline and builds the pair directlyif ... then error(...) endbranches rather thanassertcalls, which were a function call per decodeu32_to_le_bytesle_bytes_to_u32u64_to_le_bytesle_bytes_to_u64from_numberMeasured with one process per version, so the two copies never share a JIT trace.
Faster pure-Lua backend. Where there are no native or library bit ops, which includes CI's plain 5.1 entry,
band,borandbxorwalked 32 bits with twofloorcalls per bit. They now use 4-bit truth tables, so at most 8 steps, andband(a, 2^k - 1)isa % 2^k. Inputs are reduced to their low 32 bits first, so negative and oversized inputs give the same results as before. The tables are only built on the pure-Lua path.raw_bandraw_band(a, 0x7F)raw_borraw_bxorbor+ 2lshiftOld and new pure backends agree on all 342,630 comparisons across bit16, bit32 and bit64 (
band,bor,bxor, their raw forms, shifts,arshift,bnot), run on 5.1 natively and on LuaJIT and 5.4 forced onto the pure backend by hidingbit,bit32and the native-operator probe. The full selftests pass in that forced mode on LuaJIT, 5.4 and 5.3.In-range output is unchanged. An old-vs-new comparison over 290,054 cases (u32 and u64 in both byte orders, with offsets, plus
from_numberacross ±2^53) found no differences on Lua 5.1, 5.2, 5.3, 5.4 or LuaJIT 2.1.Out-of-range behaviour that changes:
Offset must be at least 1.string.unpacktreats offset 0 as 1, so without the check offset 0 would have silently read the wrong bytes on 5.3+ while raising on 5.1, 5.2 and LuaJIT. Error messages keep thefile:line:prefixassertproduced.from_number(-2^70)used to return a pair with a negative high word and now wraps to{0, 0}.from_number(-0.0)gives+0rather than-0in the high word.from_number(1/0)gives{nan, nan}rather than{inf, nan}. Both are meaningless.The offset checks and the first two
from_numbercases are pinned by new selftest vectors. The bit32 selftest also prints negative expected values as decimal.