Skip to content

Build LuaJIT w/ table.pack support#1535

Merged
NiLuJe merged 4 commits into
koreader:masterfrom
NiLuJe:master
Oct 12, 2022
Merged

Build LuaJIT w/ table.pack support#1535
NiLuJe merged 4 commits into
koreader:masterfrom
NiLuJe:master

Conversation

@NiLuJe

@NiLuJe NiLuJe commented Oct 11, 2022

Copy link
Copy Markdown
Member

It's unfortunately behind the Lua 5.2 extensions ifdef in a standard build (possibly because it's so common that it's usually implemented in Lua and monkey-patched over the table metatable?).

But it's trivial and doesn't actually poses an API issue, and it's helpful for a number of our use-cases.


This change is Reviewable

And also, enable table.pack in our build.
Note that it stores the full length in a "n" field, like old timey Lua,
but the # syntactic sugar transparently handles it, and won't return a
length truncated at the first nil ;).
@NiLuJe

NiLuJe commented Oct 11, 2022

Copy link
Copy Markdown
Member Author

ipairs will still stop at the first nil, so a sparse table packed this way needs to be walked via a for loop (if order is important, which it usually is for those ;o).

e.g.,

> t = table.pack(1, nil, 3)
> for i, v in ipairs(t) do print(i, v) end
1       1
> for i = 1, #t do print(i, t[i]) end
1       1
2       nil
3       3

(Although, generally, when you pack it's because you want to consume it unpacked as list of arguments, not walk it as a table. If you need to iterate over varargs and you're able to pass them along as-is, select is the way to do that).

@NiLuJe

NiLuJe commented Oct 11, 2022

Copy link
Copy Markdown
Member Author

LifeHack: there's nothing magical about the n field being used here (it's a remnant of old Lua semantics, IIRC).

> t = { n = 3, 1, nil, 3 }
> for i = 1, #t do print(i, t[i]) end
1       1
2       nil
3       3

Which is why this is often implemented in pure-Lua as:

table.pack = function(...)
    return { n = select("#", ...), ... }
end

@Frenzie

Frenzie commented Oct 12, 2022

Copy link
Copy Markdown
Member

It's unfortunately behind the Lua 5.2 extensions ifdef in a standard build (possibly because it's so common that it's usually implemented in Lua and monkey-patched over the table metatable?).

Should we just enable that? The stuff with break statements and whatnot sounds useful.

@NiLuJe

NiLuJe commented Oct 12, 2022

Copy link
Copy Markdown
Member Author

Should we just enable that? The stuff with break statements and whatnot sounds useful.

A few of the other things are mildly annoying and would break existing code, though, so, ultimately, maybe, but not right now ;).

@NiLuJe NiLuJe left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 4 of 4 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @NiLuJe)

@NiLuJe NiLuJe merged commit 4216c40 into koreader:master Oct 12, 2022
NiLuJe added a commit to koreader/android-luajit-launcher that referenced this pull request Oct 12, 2022
c.f., koreader/koreader-base#1535

Note that I left a couple of `{...}` constructs alone in android.lua, because some were then passing the table to `table.concat`, which *will* choke on a nil (and it gets to iterate over it in a packed vararg table).
NiLuJe added a commit to NiLuJe/koreader that referenced this pull request Oct 12, 2022
@NiLuJe

NiLuJe commented Oct 12, 2022

Copy link
Copy Markdown
Member Author

LifeHack: there's nothing magical about the n field being used here (it's a remnant of old Lua semantics, IIRC).

Well, there is a bit of weird Lua clunk involved nonetheless ;p.

> t = { 1, 2, 3, nil, 4 }
> t.n = 5
> print(unpack(t))
1       2       3
> t = { n = 5, 1, 2, 3, nil, 4 }
> print(unpack(t))
1       2       3       nil     4
> t = { 1, 2, 3, nil, 4, n = 5 }
> print(unpack(t))
1       2       3

NiLuJe added a commit to koreader/koreader that referenced this pull request Oct 12, 2022
* Iterate over varargs directly via select if possible
* Use table.pack otherwise (koreader/koreader-base#1535).
* This allows us to simplify a few Logger calls, as logger now handles nil values.
@NiLuJe

NiLuJe commented Apr 23, 2023

Copy link
Copy Markdown
Member Author

As discovered in koreader/koreader#10346, shit gets interesting when you throw more nils in there...

> t = table.pack(1, nil, 3)
> print(#t)
3
> t = table.pack(1, nil, 3, nil)
> print(#t)
1
> print(t.n)
4

Sooo, yeah, better rely on the n , field instead of the # syntactic sugar for len...

NiLuJe added a commit to NiLuJe/koreader-base that referenced this pull request Apr 23, 2023
Turns out we can't rely on the len operator in all cases,
as it may get confused depending on where the `nil`s are,
so always use the `n` field instead, as that *is* reliable.

Re: koreader#1535
NiLuJe added a commit that referenced this pull request Apr 23, 2023
Turns out we can't rely on the len operator in all cases,
as it may get confused depending on where the `nil`s are,
so always use the `n` field instead, as that *is* reliable.

Re: #1535
0xstillb pushed a commit to 0xstillb/koreader-thai that referenced this pull request May 9, 2026
* Iterate over varargs directly via select if possible
* Use table.pack otherwise (koreader/koreader-base#1535).
* This allows us to simplify a few Logger calls, as logger now handles nil values.
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.

2 participants