-
Notifications
You must be signed in to change notification settings - Fork 52
Add a new-generic Set backed by a native Lua table #477
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
Frotty
wants to merge
7
commits into
master
Choose a base branch
from
feat/lua-native-set
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
e8bd417
Add a new-generic Set backed by a native Lua table.
Frotty 820a230
Import ErrorHandling in KeyedTable.
Frotty ecbcac8
Rename Set to KeyedSet and name its fields by role.
Frotty da74841
Make KeyedSet Lua-only instead of carrying a Jass scan fallback.
Frotty 6291b15
Give a keyed set a working Jass fallback.
Frotty 46fdcd2
Attach the KeyedTable overview to nothing, as a plain comment.
Frotty 59d4153
Say that an element must leave a keyed set before it is destroyed.
Frotty File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,77 @@ | ||
| package KeyedSet | ||
| import KeyedTable | ||
|
|
||
| /** | ||
| A set with O(1) membership, for replacing a `group` used purely to answer "is this unit in here?". | ||
|
|
||
| On Lua the elements are the keys of a native Lua table, so `contains` is one hashed index and Lua | ||
| does the hashing. That is why this uses new generics (`T:`): they are erased on Lua, so the | ||
| element arrives as itself. The old `<T>` containers - `HashSet`, `HashList`, `HashMap` - erase to | ||
| `int` instead, so every element round-trips through `castTo int` and back, and an integer index | ||
| cannot be a native Lua key at all. Those types also reach the Jass hashtable natives, which the | ||
| Jass-Lua shim emulates, paying for a hashtable on a runtime that already is one. | ||
|
|
||
| On Jass it falls back to a `Table` keyed by an integer projection of the element. That is a | ||
| hashtable native per operation and much slower than the Lua path, but the semantics are the same, | ||
| so a package using this works on both backends. Element types with no stable integer key - real, | ||
| boolean, string, code, tuples - are a compile error on Jass rather than a lossy key. | ||
|
|
||
| **There is no iteration.** Enumerating a Lua table needs `pairs()`, whose order follows internal | ||
| hash layout and so differs between clients, which desyncs a lockstep game. `ipairs()` is safe but | ||
| only walks consecutive integer keys from 1, so it sees nothing in a table keyed by elements. If | ||
| you need to iterate, keep an `ArrayList` alongside and walk that, or use `SparseSet`, whose dense | ||
| half exists for exactly this. | ||
|
|
||
| **Null is not a valid element**, and **an element must be removed before it is destroyed** - see | ||
| `KeyedTable` for why the Jass fallback cannot detect a handle id which has been reused since. This | ||
| is the same discipline a `group` needs. | ||
| */ | ||
| public class KeyedSet<T:> | ||
| /** The keyed table whose keys are the elements. */ | ||
| private int keyTable = 0 | ||
| /** Tracked rather than measured - counting a Lua table would mean iterating it. */ | ||
| private int count = 0 | ||
|
|
||
| construct() | ||
| keyTable = keyedTableCreate() | ||
|
|
||
| ondestroy | ||
| keyedTableDestroy(keyTable) | ||
|
|
||
| /** Adds `value`. Returns whether it was newly inserted. */ | ||
| function add(T value) returns boolean | ||
| if keyedTableContains(keyTable, value) | ||
| return false | ||
| keyedTableAdd(keyTable, value) | ||
| count += 1 | ||
| return true | ||
|
|
||
| /** Whether `value` is present. */ | ||
| function contains(T value) returns boolean | ||
| return keyedTableContains(keyTable, value) | ||
|
|
||
| /** Removes `value`. Returns whether it was present. */ | ||
| function remove(T value) returns boolean | ||
| if not keyedTableContains(keyTable, value) | ||
| return false | ||
| keyedTableRemove(keyTable, value) | ||
| count -= 1 | ||
| return true | ||
|
|
||
| function size() returns int | ||
| return count | ||
|
|
||
| function isEmpty() returns boolean | ||
| return count == 0 | ||
|
|
||
| /** | ||
| * Empties the set. | ||
| * | ||
| * Replaces the table rather than clearing it in place: emptying a Lua table means visiting | ||
| * its keys, and that needs `pairs()`. The old table is destroyed, which on Jass returns its | ||
| * `Table` to the pool and on Lua leaves it to the collector. | ||
| */ | ||
| function clear() | ||
| keyedTableDestroy(keyTable) | ||
| keyTable = keyedTableCreate() | ||
| count = 0 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,73 @@ | ||
| package KeyedSetTests | ||
|
|
||
| import KeyedSet | ||
|
|
||
| class Marker | ||
| int id | ||
|
|
||
| construct(int id) | ||
| this.id = id | ||
|
|
||
| @Test | ||
| function testAddAndMembership() | ||
| let set = new KeyedSet<int>() | ||
| set.contains(4).assertFalse() | ||
| set.add(4).assertTrue() | ||
| set.add(9).assertTrue() | ||
| set.contains(4).assertTrue() | ||
| set.contains(9).assertTrue() | ||
| set.contains(5).assertFalse() | ||
| set.size().assertEquals(2) | ||
| destroy set | ||
|
|
||
| @Test | ||
| function testAddIsIdempotent() | ||
| let set = new KeyedSet<int>() | ||
| set.add(7).assertTrue() | ||
| set.add(7).assertFalse() | ||
| set.size().assertEquals(1) | ||
| set.contains(7).assertTrue() | ||
| destroy set | ||
|
|
||
| @Test | ||
| function testRemoval() | ||
| let set = new KeyedSet<int>() | ||
| set.add(1) | ||
| set.add(2) | ||
| set.remove(1).assertTrue() | ||
| set.contains(1).assertFalse() | ||
| set.contains(2).assertTrue() | ||
| set.size().assertEquals(1) | ||
| // Removing something absent is not an error and does not move the count. | ||
| set.remove(1).assertFalse() | ||
| set.size().assertEquals(1) | ||
| destroy set | ||
|
|
||
| @Test | ||
| function testClear() | ||
| let set = new KeyedSet<int>() | ||
| set.add(1) | ||
| set.add(2) | ||
| set.clear() | ||
| set.isEmpty().assertTrue() | ||
| set.size().assertEquals(0) | ||
| set.contains(1).assertFalse() | ||
| set.add(1).assertTrue() | ||
| set.size().assertEquals(1) | ||
| destroy set | ||
|
|
||
| /** Reference identity, which is what a group-style membership set wants. */ | ||
| @Test | ||
| function testReferenceIdentity() | ||
| let set = new KeyedSet<Marker>() | ||
| let a = new Marker(1) | ||
| let b = new Marker(1) | ||
| set.add(a).assertTrue() | ||
| set.contains(a).assertTrue() | ||
| // Same contents, different object: a distinct member. | ||
| set.contains(b).assertFalse() | ||
| set.add(b).assertTrue() | ||
| set.size().assertEquals(2) | ||
| destroy set | ||
| destroy a | ||
| destroy b |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,89 @@ | ||
| package KeyedTable | ||
| import Table | ||
|
|
||
| // Keyed membership: a table whose keys are the elements themselves. | ||
| // | ||
| // These are compiler intrinsics, and each backend gets the representation that suits it. | ||
| // | ||
| // On **Lua** every operation lowers to a single table operation - `{}`, `t[k] = true`, | ||
| // `t[k] ~= nil`, `t[k] = nil` - so membership costs one hashed index and Lua does the hashing. | ||
| // The key is a `T:` type parameter, which new generics erase on Lua rather than routing through | ||
| // `castTo int` the way the old `<T>` containers do, so the element arrives as itself and becomes | ||
| // the table key directly. An integer index would defeat native hashing entirely. | ||
| // | ||
| // On **Jass** there is no hashing, so the bodies below run instead: the element is projected to an | ||
| // integer key by `wurstKeyOf` and stored in a `Table`. That costs a hashtable native per operation, | ||
| // which is far slower than the Lua path - but it works, which is what a fallback has to do. | ||
| // | ||
| // **There is no iteration.** Enumerating a Lua table needs `pairs()`, whose order follows internal | ||
| // hash layout and so differs between clients, which desyncs a lockstep game. Anything that must be | ||
| // iterated needs a separately maintained insertion-ordered array - see `SparseSet`. | ||
| // | ||
| // **Null is not a valid key.** On Lua `nil` cannot be a table key at all, so adding one is a | ||
| // runtime error there; 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 it. | ||
| // | ||
| // **Remove a handle before destroying it.** Warcraft reuses handle ids. On Jass a key is that id | ||
| // and the table stores only that the id is present, so a new handle which inherits the id of a | ||
| // destroyed member is reported as a member. Validating that would mean storing the element next to | ||
| // its key, and `Table` offers no way to store an arbitrary `T` - only `saveUnit`, `saveItem` and | ||
| // the rest, one per concrete type. That is the reason `SparseSet` asks for a `SparseSetKey` and | ||
| // keeps its elements in a dense list: it can compare the stored element and this cannot. | ||
| // | ||
| // The discipline is the same one a `group` needs, and the same one `SparseSet` documents: take an | ||
| // element out before it is destroyed. What is not claimed is that the two backends agree about a | ||
| // member which was destroyed and whose id has since been reused. | ||
|
|
||
| /** A new, empty keyed table. */ | ||
| @compilerintrinsic public function keyedTableCreate() returns int | ||
| return (new Table()) castTo int | ||
|
|
||
| /** Adds `key`. Adding a key that is already present has no effect. */ | ||
| @compilerintrinsic public function keyedTableAdd<T:>(int keyedTable, T key) | ||
| (keyedTable castTo Table).saveBoolean(wurstKeyOf(key), true) | ||
|
|
||
| /** Whether `key` is present. */ | ||
| @compilerintrinsic public function keyedTableContains<T:>(int keyedTable, T key) returns boolean | ||
| return (keyedTable castTo Table).loadBoolean(wurstKeyOf(key)) | ||
|
|
||
| /** Removes `key`. Removing a key that is absent has no effect. */ | ||
| @compilerintrinsic public function keyedTableRemove<T:>(int keyedTable, T key) | ||
| (keyedTable castTo Table).removeBoolean(wurstKeyOf(key)) | ||
|
|
||
| /** | ||
| Frees the keyed table. | ||
|
|
||
| On Lua this is a no-op: the table is garbage once the last reference is dropped. On Jass it | ||
| releases the `Table` instance, which comes from a finite pool - without this, every discarded | ||
| keyed structure would burn one permanently. | ||
| */ | ||
| @compilerintrinsic public function keyedTableDestroy(int keyedTable) | ||
| destroy (keyedTable castTo Table) | ||
|
|
||
| // --- | ||
| // Key projection. Compiler-owned: these exist so the Jass bodies above have something to call, | ||
| // and are not meant to be called directly. | ||
| // --- | ||
|
|
||
| /** | ||
| The integer key of `value` on Jass. | ||
|
|
||
| A `T:` type parameter cannot be projected to an integer in Wurst - that is why `SparseSet` has to | ||
| ask its caller for a `SparseSetKey`. The compiler fills this in after generic elimination, when | ||
| each specialisation's element type is concrete, choosing one of the projections below. Element | ||
| types with no stable integer key - real, boolean, string, code, tuples - are rejected there with | ||
| a compile error rather than keyed on something lossy. | ||
|
|
||
| On Lua this is never reached: the operations above are replaced wholesale and the element is its | ||
| own key, so there is no projection to make. | ||
| */ | ||
| @compilerintrinsic public function wurstKeyOf<T:>(T value) returns int | ||
| return 0 | ||
|
|
||
| /** The projection chosen for ints and for class instances, which are integers by that point. */ | ||
| @compilerintrinsic public function keyOfInt(int value) returns int | ||
| return value | ||
|
|
||
| /** The projection chosen for handles, whose id is their key. */ | ||
| @compilerintrinsic public function keyOfHandle(handle value) returns int | ||
| return GetHandleId(value) | ||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When a unit or destructable is destroyed while still present and Warcraft later reuses its handle ID, the Jass table still contains
trueunder that integer, socontains(newHandle)incorrectly returns true andadd(newHandle)returns false; the Lua backend instead keys by the handle itself. The existingSparseSetexplicitly compares the stored handle to prevent this inheritance (SparseSet.wurstlines 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Tablehas no way to store an arbitraryT— it offerssaveUnit,saveItem,saveDestructable,saveWidget,savePlayerand so on, one per concrete type, and nothing generic. That is exactly whySparseSetasks its caller for aSparseSetKeyand keeps a denseArrayList<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
groupneeds and the same oneSparseSetalready documents.Documented on both
KeyedTableandKeyedSetin 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
groupmust 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.