RFC: Extended Collection Types #2
Conversation
|
|
||
| world.log << bar["test"] // Runtime error, key not found | ||
|
|
||
| bar.contains_key("test") // Returns FALSE |
There was a problem hiding this comment.
Would it not be more dmish to do "test" in bar?
There was a problem hiding this comment.
Presumably we would support both.
There was a problem hiding this comment.
I just don't see the need for an extra function if there's already a byondism. Keep in mind .Find exists too.
There was a problem hiding this comment.
I feel like just straight reusing Find() could be a bit ambiguous but FindKey() would be better than contains_key().
There was a problem hiding this comment.
Find by itself could be confused with value searching
| ``` | ||
|
|
||
| ### ODList | ||
| Identical to BYOND `list()` but with no support for associative lists. This facilitates various internal optimizations over using `list()`. |
There was a problem hiding this comment.
Can't we just optimise lists to work well when not associative? Like I'm a little bit sure we already do that.
There was a problem hiding this comment.
We already do as much as we can, but further improvements can be made if we can guarantee a list will never become a dict and vice-versa.
| world.log << bar[1] // Runtime error, dicts can't be numerically indexed | ||
| ``` | ||
|
|
||
| `dict.keys` and `dict.values` will return a new `array()` of the dictionary's keys and values, respectively. Modifying these will not modify the existing dictionary. |
There was a problem hiding this comment.
If its not going to be linked, maybe it should return a constant list instead.
There was a problem hiding this comment.
I have no strong opinion so I'll see if anyone else weighs in. I'm not sure what value a const-list has over an array though?
There was a problem hiding this comment.
It might be better to define these as being some kind of iterator so that the implementation of keys/values can be allocation-free.
Co-authored-by: Amy <[email protected]>
|
Higher level design question, would introducing the concept of mutation-locking (i.e. rendering it impossible to change the elements of the list, so preventing shallow mutation. deep mutation i.e. mutation within elements isn't really preventable with DM's type model) these types be feasible? in other words could read-only collections be introduced as well. |
|
|
||
| Example usage: | ||
| ``` | ||
| var/odlist/foo = odlist("a","b","c") |
There was a problem hiding this comment.
odlist seems rather awkward of a name. something like dynarray or collection seems a little bit more fitting.
There was a problem hiding this comment.
collection is too generic and dynarray is cringe.
The rationale here is that odlist prevents namespace pollution and codebases can wrap it in a macro if they want to rename it.
There was a problem hiding this comment.
Couldnt think of a better name; but that seems good albeit smidge weird.
|
|
||
| bar.get_value("test") // Returns null, since the key doesn't exist | ||
|
|
||
| world.log << bar[1] // Compile-time error, dicts can't be numerically indexed |
There was a problem hiding this comment.
Why are we not allowing dictionaries to have a numerical key? That's how it works with the existing lists, but this is an annoying limitation with the current system and if there is going to be a specific dictionary type, I see no reason why we have to block of numerical keys (since we can now identify between what the user really wants based on whether or not they ask for a dict or an array)
There was a problem hiding this comment.
Dictionaries aren't ordered in C# and we're just exposing the C# type to DM.
There was a problem hiding this comment.
I mean, what if we have cases where we want to actually have an numerical key. Within current byond, we have to cast the number to a string and store it in the dictionary.
An example code of something that I think would be really useful to have, that would be an improvement over current byond:
var/dict/player_locations = dict(4 = dict(8 = odlist(new /mob)), 9 = dict(4 = odlist(new /mob)))
// Find the player at 4, 8
var/odlist/players = player_locations[4][8]
There was a problem hiding this comment.
Even without typed collections I would expect a dictionary type to have arbitrarily-typed DreamValue keys rather than just strings.
This would give similar reasons to use dict over list as there are to use JavaScript's Map type over normal JS objects.
| ``` | ||
|
|
||
| ### Dictionary | ||
| A collection of key-value pairs. Equivalent to existing associative lists, and will serve as the internal implementation of BYOND's planned `alist()` feature. |
There was a problem hiding this comment.
Can you enumerate over dictionary types without using .keys or .values?
| var/const/array/foo = array("a","b","c") | ||
| foo[1] = "z" // Compile-time error, cannot modify constant collection |
There was a problem hiding this comment.
I think this needs some explanation of how it is going to work with relation to casting.
My assumption is:
- Any non-constant array can be cast into a constant array (or provided as a parameter to a function that takes constant arrays)
- Any constant array cannot be cast into a non-constant array
If the above was the case, then how would the following work?
var/array/test = array("a", "b")
var/const/array/constant_test = test
test[1] = "c"
world.log << constant_test[1]
Would constant arrays have to be compile time constant (which would seriously limit their use imo), would they allocate new memory on assign (which could be very inefficient), would they accept that they can change if the underlying array they are based on was changed, or would they do something else thats more complex (something more advanced like trying to figure out at compile time if something is being modified later down the line, or allocating new memory if and only if the array that it was based on has the opportunity to be modified)?
| world.log << foo.dequeue() // Removes "a" from `foo` and prints "a" | ||
| ``` | ||
|
|
||
| A `queue.to_array()` helper will also be provided, and returns the queue's elements as a shallow-copied `array()`. |
There was a problem hiding this comment.
Would this be implicitly done if you do
for (var/thing in foo)
or would you be forced to do
for (var/thing in foo.to_array())
| world.log << foo.dequeue() // Removes "a" from `foo` and prints "a" | ||
| ``` | ||
|
|
||
| A `queue.to_array()` helper will also be provided, and returns the queue's elements as a shallow-copied `array()`. |
There was a problem hiding this comment.
I like it being a method on queue, although it could potentially be more 'DMy' if it was something like array(queue) since all the current conversion procs in DM are global procs that take the thing to convert as a parameter. I think this shoulsd have some discussion around it.
There was a problem hiding this comment.
I like the method on queue as well. Would be nice to have it that way for the other conversion procs as well. A stupid compromise can just be having both but that's yucky.
|
Everyone in coderbus seems to prefer |
|
Officially voicing my vote for `vec`.
…On Mon, May 6, 2024, 1:08 p.m. ike709 ***@***.***> wrote:
Everyone in coderbus seems to prefer vec over odlist so barring any
feedback to the contrary I'll be making that change when I have time
tomorrow(?) (and address the remainder of outstanding feedback).
—
Reply to this email directly, view it on GitHub
<#2 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AB6LA6UY6Q7YUQ653QOIPI3ZA62HXAVCNFSM6AAAAABG4OLCCWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAOJWGUYTQMBVGY>
.
You are receiving this because you are subscribed to this thread.Message
ID: ***@***.***>
|
officially seconding the vote for |
|
Third ditto, even if it's not resizeable or anything I still think it's a much more familiar name. It can also be aliased to anything people want, since I'm not aware of any largely used common procs named vec. |
|
I actually prefer |
|
|
||
| Generally speaking, the helper procs from `list()` will also operate on other collections where applicable (e.g. `list.Join()` and `array.Join()` would be equivalent). A few collections will implement additional helpers as noted below. | ||
|
|
||
| **Note:** Initializing OpenDream's collection types with a fixed size, for example `var/array/foo[10]`, will initialize it with a `capacity` of 10 but *will not* insert any `null` elements; the actual contents of the collection will be empty. This is **breaking behavior** from BYOND lists such as `var/list/L[10]`, which inserts 10 `null` elements into the list. BYOND lists in OpenDream will preserve this behavior for compatibility. Otherwise, see the "Collection Capacity API" subsection below for more information. |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
|
||
| **Note:** Initializing OpenDream's collection types with a fixed size, for example `var/array/foo[10]`, will initialize it with a `capacity` of 10 but *will not* insert any `null` elements; the actual contents of the collection will be empty. This is **breaking behavior** from BYOND lists such as `var/list/L[10]`, which inserts 10 `null` elements into the list. BYOND lists in OpenDream will preserve this behavior for compatibility. Otherwise, see the "Collection Capacity API" subsection below for more information. |
There was a problem hiding this comment.
Also, would it be reasonable to have something like
var/array/arr[2, 3]
arr = { { 1, 2, 3 }, { 4, 5, 6 } }
(basically int[,] multiDimensionalArray = new int[2, 3];)
if it's decided to keep the var/vec/foo[10] syntax around
| world.log << foo.dequeue() // Removes "a" from `foo` and prints "a" | ||
| ``` | ||
|
|
||
| A `queue.to_array()` helper will also be provided, and returns the queue's elements as a shallow-copied `array()`. |
There was a problem hiding this comment.
I like the method on queue as well. Would be nice to have it that way for the other conversion procs as well. A stupid compromise can just be having both but that's yucky.
| A fixed-length collection of elements. Almost identical to `list()` except it cannot be resized. | ||
|
|
There was a problem hiding this comment.
Just to be clear "almost identical" also means no assoc behaviour for this yes
+1 |
|
|
||
| Generally speaking, the helper procs from `list()` will also operate on other collections where applicable (e.g. `list.Join()` and `array.Join()` would be equivalent). A few collections will implement additional helpers as noted below. | ||
|
|
||
| **Note:** Initializing OpenDream's collection types with a fixed size, for example `var/array/foo[10]`, will initialize it with a `capacity` of 10 but *will not* insert any `null` elements; the actual contents of the collection will be empty. This is **breaking behavior** from BYOND lists such as `var/list/L[10]`, which inserts 10 `null` elements into the list. BYOND lists in OpenDream will preserve this behavior for compatibility. Otherwise, see the "Collection Capacity API" subsection below for more information. |
There was a problem hiding this comment.
How is it possible for a "fixed-length" collection of elements (the array) to have a capacity of 10 but contain no elements? I believe it should be populated with 10 nulls.
There was a problem hiding this comment.
How is it possible for a "fixed-length" collection of elements (the array) to have a
capacityof 10 but contain no elements? I believe it should be populated with 10 nulls.
This has to do with memory allocation, and the size of the memory allocated for the collection vs its actual contents. Capacity of 10 means it has space set aside for 10 elements, but may have any number of elements between 0 and 10.
This is what any modern language does, and DM itself likely does it internally without telling the user about it (allowing the user to see it means they can give the VM an advisory on how much memory they expect to need, and it can allocate it all ahead of time.)
There was a problem hiding this comment.
There was a problem hiding this comment.
yeah but my point is that an array can not be resized, so it doesn't make sense for arrays specifically to have a capacity (at least, a capacity differing from its size)
There was a problem hiding this comment.
Thinking about this more, I really don't see how it makes sense for other containers either. Do any other languages exist with this kind of syntax for specifying the initial capacity of a container?
Is the [10] expected to be part of the type? I don't think that initial capacity should have anything to do with the type of the variable. I'd argue that this syntax should be entirely dropped for these new data types and moved to the right-hand-side of the variable declaration inside a default expression (somehow, I can't think of the nicest approach), where it probably belongs. This would also allow the feature to be used in places that are not variable declarations.
tbh BYOND's current behaviour makes far more sense than using this for capacity, although it's still weird.
The syntax maybe makes sense for non-reassignable array members, assuming it sets the size rather than initial capacity, but I don't think we need those.
There was a problem hiding this comment.
hm. I'm no longer sure either, can you bring this up in the discord?
|
|
||
| bar.get_value("test") // Returns null, since the key doesn't exist | ||
|
|
||
| world.log << bar[1] // Compile-time error, dicts can't be numerically indexed |
There was a problem hiding this comment.
Even without typed collections I would expect a dictionary type to have arbitrarily-typed DreamValue keys rather than just strings.
This would give similar reasons to use dict over list as there are to use JavaScript's Map type over normal JS objects.
| world.log << bar[1] // Runtime error, dicts can't be numerically indexed | ||
| ``` | ||
|
|
||
| `dict.keys` and `dict.values` will return a new `array()` of the dictionary's keys and values, respectively. Modifying these will not modify the existing dictionary. |
There was a problem hiding this comment.
It might be better to define these as being some kind of iterator so that the implementation of keys/values can be allocation-free.
|
I've slept on it and decided we should do namespaces first to give us more freedom to name things whatever we want without worrying about namespace pollution and creating any conflicts with existing code. This also saves me the trouble of having to revisit this RFC after the addition of namespaces and needing to move all of the collections code over into its own namespace. I've marked this as a draft and I'll revisit it once namespaces have been sorted out, with the review window being reset to the normal 2 weeks. |
Rather than doing a single gigantic RFC for all aspects of an improved type system for DM, I'm breaking it up piecemeal.
This RFC focuses on adding several new collection types. A future RFC will implement support for typed collections (e.g. an array of strings).
Since I'm pretty much directly exposing C# collections to DM, I wasn't really sure how much detail was needed or how much of it would be redundant. I can elaborate as-requested.