Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 820a230572
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
e1cc0ee to
da74841
Compare
Both types were Lua-only, erroring on Jass because a `T:` type parameter cannot be projected to an integer key in Wurst. The compiler now supplies that projection after generic elimination, so KeyedTable gets real bodies: the element is keyed through wurstKeyOf and stored in a Table. That is a hashtable native per operation and much slower than the Lua path, where each operation is a single index, but the semantics match, so a package using a keyed set works on both backends. A keyed set is now disposable. Replacing the table on clear() is still how emptying works - emptying a Lua table needs pairs(), which desyncs - but the old one is destroyed rather than orphaned, and ondestroy frees the set's own, so the Jass fallback no longer burns a Table instance per clear and per set. The tests drop their isLua guards and now run on both backends.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 46fdcd24c7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| /** The projection chosen for handles, whose id is their key. */ | ||
| @compilerintrinsic public function keyOfHandle(handle value) returns int | ||
| return GetHandleId(value) |
There was a problem hiding this comment.
When a unit or destructable is destroyed while still present and Warcraft later reuses its handle ID, the Jass table still contains true under that integer, so contains(newHandle) incorrectly returns true and add(newHandle) returns false; the Lua backend instead keys by the handle itself. The existing SparseSet explicitly compares the stored handle to prevent this inheritance (SparseSet.wurst lines 39-45 and 145-148), so the Jass fallback must retain enough identity to validate a reused ID rather than storing only a boolean.
AGENTS.md reference: AGENTS.md:L12-L12
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
The Jass half of this is real and I have documented it as a contract rather than claimed to fix it. Flagging that choice rather than resolving quietly, because it is a design decision.
Why not fixed. Validating a reused id means storing the element beside its key so it can be compared. Table has no way to store an arbitrary T — it offers saveUnit, saveItem, saveDestructable, saveWidget, savePlayer and so on, one per concrete type, and nothing generic. That is exactly why SparseSet asks its caller for a SparseSetKey and keeps a dense ArrayList<T>: it can compare the stored element, and a keyed table storing one boolean cannot. Adding that here would rebuild SparseSet's shape and give up the reason this type exists.
What I am not claiming. I have not verified how Warcraft represents a handle in Lua, so I will not assert either that the backends agree here or that they differ. What the docs now say is narrower and checkable: an element must be removed before it is destroyed, and membership of a destroyed element whose id has since been reused is not defined. That is the same discipline a group needs and the same one SparseSet already documents.
Documented on both KeyedTable and KeyedSet in 59d4153, including why the fallback cannot do better and where to go instead when identity across recycling matters.
If the view is that a set replacing a group must survive recycling, that is a different type — SparseSet's shape, with the allocation and indirection that come with it — and worth deciding deliberately rather than folding in here.
1 participant
A set with O(1) membership, meant to replace a
groupused purely to answer "is this unit in here?".Why a new type rather than
HashSetThe old
<T>containers erase toint, so every element round-trips throughcastTo intand back. On Lua that is doubly wrong: the round-trip costs, and an integer index cannot be a native table key at all, so the runtime's own hashing is thrown away. Those types also bottom out in the Jass hashtable natives, which the Jass-Lua shim emulates — paying for a hashtable on a runtime that already is one.KeyedSet<T:>uses new generics, which are erased on Lua rather than cast, so the element arrives as itself and becomes the table key directly.What each backend gets
On Lua, every operation is a single table operation:
and the call sites hand over the unit itself — no index round-trip.
On Jass there is no hashing, so
KeyedTableruns the bodies in this PR: the element is projected to an integer key bywurstKeyOfand stored in aTable. That is a hashtable native per operation and much slower, but the semantics match, so a package using a keyed set works on both backends. Element types with no stable integer key — real, boolean, string, code, tuples — are a compile error there rather than a lossy key.Lifetime
clear()replaces the table rather than emptying it: emptying a Lua table means visiting its keys, which needspairs(), whose order follows internal hash layout and so differs between clients — that desyncs a lockstep game. The old table is destroyed rather than orphaned, andondestroyfrees the set's own, so the Jass fallback does not burn aTableinstance perclear()and per set.No iteration
Deliberate, same reason.
ipairs()is safe but only walks consecutive integer keys from 1, so it sees nothing in a table keyed by elements. Anything that must be iterated needs a separately maintained insertion-ordered array —SparseSet's dense half exists for exactly that.Null
Not a valid element.
nilcannot be a Lua table key at all, and on Jass it would collide with the key reserved for absence. Neither backend is asked to invent a meaning for it, and no check is added on the membership path to look for one.Ordering — must land after wurstscript/WurstScript#1305
Not a preference. Before this PR the
KeyedTableoperations callederror(LUA_ONLY)on Jass, so an unlowered call failed loudly. Giving them real Jass bodies means an unlowered call now silently computes the wrong answer instead.That path is reachable today. Stack-trace injection appends a parameter to every affected function, and on Lua every non-native function is affected, so the exact signatures the keyed-table operations are recognised by stop matching and the lowering does not happen. The Jass bodies then run on Lua, where
wurstKeyOfanswers with its placeholder0— every element shares one key, and a set reports that it contains anything it is asked about. A release build emits stack traces by default, so this is the common case.wurstscript/WurstScript#1305 fixes that ordering, and also makes
keyedTableDestroycost nothing on Lua. Merging this first would ship a silent correctness bug.Verification
Beyond this repo's own tests (which now run on both backends — the
isLuaguards are gone), the compiler side was checked end to end against this branch:KeyedSet<unit>add/contains/remove/size/clear/destroy on Jass, and on Lua that the emitted membership is a single index with the unit as the key.