-
Notifications
You must be signed in to change notification settings - Fork 29
Write down the natively keyed store design before building it #1255
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
base: master
Are you sure you want to change the base?
Changes from 1 commit
a703d89
a240362
d26c0dc
de23cc3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,6 +17,62 @@ The container these bounds were added for now compiles and runs against the stan | |
| Jass, and compiles on Lua (#1239). What is left is generality around it rather than the feature | ||
| itself, and one gap in what the suite can see. | ||
|
|
||
| 27. **A natively keyed store on Lua, selected by a type class.** Decided with the repo owner: | ||
| **the native path is taken only where the key's equality is identity**, and a type class says | ||
| which keys those are. Everything else keeps today's probing on both targets. | ||
|
|
||
| Why it is worth doing. `FastHashMap` does its own hashing and linear probing on both targets, but | ||
| a Lua table already is a hash map: `t[key] = value` would let Lua hash, and would lift the fixed | ||
| `FASTHASHMAP_CAPACITY`/`FASTHASHMAP_MAX_INSTANCES` limits there entirely. It would also get string | ||
| keys off `StringHash`, which this library documents as unusable for the purpose - case insensitive | ||
| (`String.wurst:81`, so `a` and `A` share a key), collapsing every partial multibyte slice to one | ||
| constant (`MultibyteDiagnostics`), undocumented and changed between game versions, and **not | ||
| emulated by the interpreter**, so `FastHashMapTests.testStringKeys` passing says nothing about the | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
At this commit, Useful? React with 👍 / 👎. |
||
| game. Other containers - a set, a memo cache, adjacency maps - then build on the one store. | ||
|
|
||
| Why the type class is load bearing rather than incidental. A Lua table matches keys by raw | ||
| identity, which for a class is reference identity. An instance whose `equals` is structural - | ||
| `Hashable<vec2>` comparing components - would therefore have Jass treat two equal-valued keys as | ||
| one key and Lua treat them as two, silently, from one program. So the native path is sound only | ||
| for `int`, `real`, `string`, `boolean` and reference-keyed classes. A second bound states that: | ||
|
Comment on lines
+38
to
+39
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
For any AGENTS.md reference: AGENTS.md:L217-L221 Useful? React with 👍 / 👎. |
||
|
|
||
| public interface RawKeyed<T:> // no requirements; a promise that equality is identity | ||
|
|
||
| class FastHashMap<K: Hashable> // probing on both targets, any key | ||
| class FastHashMap<K: Hashable and RawKeyed> // t[k] on Lua | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Wurst does not overload type definitions based on generic bounds, so declaring both of these Useful? React with 👍 / 👎. |
||
|
|
||
| which is what the `and` bound is for, and what makes the choice of representation a property of | ||
| the key type rather than of the container. | ||
|
|
||
| Groundwork already established, so the next attempt does not have to find it again: | ||
|
|
||
| - Intrinsics are declared in Wurst with `@compilerintrinsic` in `wurst/_wurst/MagicFunctions.wurst` | ||
| and bounds parse there; `wurstNewInstance<T:>() returns T` is the shape to copy. Recognition is | ||
| by name plus `!AttrFuncDef.hasApplicableUserFunction(call)` in `CompilerIntrinsics`. | ||
| - `ImTranslator.isLuaTarget()` is available during Wurst-to-IM lowering, which is what makes this | ||
| feasible: the intrinsic can lower differently per target rather than needing dead-branch | ||
| elimination to have happened first. An `if isLua` guard alone does not help, because folding | ||
| runs after translation and both branches are lowered. | ||
| - A Wurst array access already lowers to a plain `t[i]` on Lua | ||
| (`lua.translation.ExprTranslation.translateArrayAccessRaw` builds `LuaExprArrayAccess`). Nothing | ||
| in the backend needs changing; the only obstacle is that the language requires an `int` index, | ||
| and typechecking is target independent, so relaxing it for Lua alone would let a program compile | ||
| for one target and fail on the other. | ||
| - `ImTranslator.imError(trace, message)` gives a runtime error call, for the Jass lowering of an | ||
| intrinsic that has no Jass meaning. `ImStatementExpr` is available for pairing statements with a | ||
| value. | ||
|
|
||
| Left to settle. The read is straightforward - `wurstKeyedRead(store, key)` lowering to | ||
| `store[key]`. The write is the open question: as an `ExprFunctionCall` it must lower to an | ||
|
Comment on lines
+73
to
+74
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The read is not yet straightforward for multiple map instances because this note only identifies existing Wurst arrays as Lua tables, while arrays cannot be function parameters ( Useful? React with 👍 / 👎. |
||
| `ImExpr`, while what it wants to be is an `ImSet` to an array access. Either wrap it in an | ||
| `ImStatementExpr` with a discarded value, or expand it at AST level after validation the way | ||
| `wurstMapFields` assigns back to fields, which is a different mechanism and may be the cleaner | ||
| one. Settle that before writing the surface. | ||
|
|
||
| Blocks `WurstStdlib2#468`, deliberately: shipping `FastHashMap` first would commit | ||
| `FASTHASHMAP_CAPACITY`, `isFull()` and `Hashable.hash` to the public API when the Lua path makes | ||
| all three meaningless on that target. | ||
|
Comment on lines
+106
to
+108
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This leaves the repository's canonical backlog claiming that item 27 deliberately blocks WurstStdlib2#468, even though Useful? React with 👍 / 👎. |
||
|
|
||
| 22. **The library's own tests do not run on Lua.** They run on the interpreter now — all 460 of | ||
| them, collected by importing every package in the checkout whose name ends in `Tests`. That | ||
| half is done; this is the other one. | ||
|
|
||
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
Vis a class or handle type andput(key, null)is called, this lowering becomest[key] = nil, which deletes the entry in Lua. The probing implementation tracks occupancy separately, so the same call still affectshasandsizeeven thoughgetreturns null; the native representation therefore needs a sentinel or separate presence table to preserve backend parity.AGENTS.md reference: AGENTS.md:L217-L221
Useful? React with 👍 / 👎.