-
Notifications
You must be signed in to change notification settings - Fork 2
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
RFC: Extended Collection Types #2
base: main
Are you sure you want to change the base?
Changes from 2 commits
8d22dc0
cc47e38
9a61474
55a5e08
3848870
3bfd6b9
9d4b70b
c040baa
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 |
---|---|---|
@@ -0,0 +1,169 @@ | ||
# Extended Collection Types | ||
|
||
| Lead Author(s) | Implemented | GitHub Links | | ||
|---|---|---| | ||
| ike709 | :x: No | TBD | | ||
|
||
## Overview | ||
|
||
Currently, the only collection type in BYOND is the `list()` type, which doubles as both a dynamically resizable list and also as a dictionary. This RFC proposes exposing several additional collection types directly from C# to DM. | ||
|
||
Note that typed collections (e.g. an array of text strings) will be implemented as a future RFC; this RFC primarily focuses on *which* collection types to add in the first place. | ||
|
||
## Motivation | ||
|
||
The implementation of BYOND lists is highly inefficient in OpenDream as we can't just back it with a C# list or a C# dictionary. Furthermore, other collection types would offer performant convenience features for developers that they do not need to implement themselves, such as performant hashsets or stacks. | ||
|
||
## Design | ||
|
||
The current `list()` type will remain unchanged for BYOND compatibility, with the other types being added *in addition to* these legacy lists. | ||
|
||
The following collection types will essentially be exposed directly from C# to DM through a minimal wrapper API: | ||
|
||
- **Array**: A fixed-length collection of elements. | ||
- **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. | ||
- **HashSet**: A collection that contains no duplicate elements. | ||
- **ODList**: A dynamically resizable collection that is purely a list with none of the assoc-list functionality that BYOND lists have. | ||
- **Queue**: A first-in first-out (FIFO) collection. | ||
- **Stack**: A last-in first-out (LIFO) collection. | ||
|
||
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. | ||
Comment on lines
+31
to
+32
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. Also, would it be reasonable to have something like
(basically int[,] multiDimensionalArray = new int[2, 3];) 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. How is it possible for a "fixed-length" collection of elements (the array) to have a 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 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. hm. I'm no longer sure either, can you bring this up in the discord? |
||
|
||
### Array | ||
|
||
A fixed-length collection of elements. Almost identical to `list()` except it cannot be resized. | ||
|
||
Comment on lines
+36
to
+37
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. Just to be clear "almost identical" also means no assoc behaviour for this yes |
||
Example usage: | ||
``` | ||
var/array/foo = array("a","b","c") | ||
foo += "z" // Compile-time error, cannot resize arrays | ||
|
||
world.log << foo.Join() // Prints "abc" | ||
|
||
foo[1] = "z" // Valid modification | ||
``` | ||
|
||
### 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you enumerate over dictionary types without using 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. Great question! |
||
|
||
Example usage: | ||
``` | ||
var/dict/bar = array("a" = 1,"b" = 2,"c" = 3) | ||
ike709 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
bar += "d" // Compile-time error, no value specified | ||
|
||
bad["d"] = 4 // Valid modification | ||
ike709 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
world.log << bar["c"] // Prints 3 | ||
|
||
world.log << bar["test"] // Runtime error, key not found | ||
|
||
bar.contains_key("test") // Returns FALSE | ||
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. Would it not be more dmish to do 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. Presumably we would support both. 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. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I feel like just straight reusing 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. Find by itself could be confused with value searching |
||
|
||
bar.get_value("test") // Returns null, since the key doesn't exist | ||
|
||
world.log << bar[1] // Runtime error, dicts can't be numerically indexed | ||
ike709 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
``` | ||
|
||
`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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If its not going to be linked, maybe it should return a constant list instead. 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. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It might be better to define these as being some kind of iterator so that the implementation of keys/values can be allocation-free. |
||
|
||
`dict.contains_key(key)` will return `TRUE` or `FALSE` depending on if the `key` is present in the dictionary. | ||
|
||
`dict.get_value(key)` will return the value of the specified `key` or `null` if the key is not present in the dictionary. | ||
|
||
### HashSet | ||
Essentially identical to a list, except it cannot contain duplicates. Inserting an element that is already present in the hashset is not an error and instead does nothing. | ||
ike709 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
Direct insertion (e.g. `hashset += "foo"`) is supported, but `hashset.add(element)` will return `TRUE` or `FALSE` depending on if the element already existed in the hashset. | ||
|
||
Example usage: | ||
``` | ||
var/hashset/foo = hashset("a","b","c") | ||
|
||
foo += "c" // Valid but nothing happens since `foo` already contains "c" | ||
|
||
foo["c"] = 5 // Compile-time error, hashsets cannot be associative | ||
|
||
world.log << foo.add("c") // Prints FALSE and inserts nothing since `foo` already contains "c" | ||
|
||
world.log << foo.add("d") // Prints TRUE and inserts "d" into `foo` | ||
world.log << foo.Join() // Prints "abcd" | ||
ike709 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
``` | ||
|
||
### 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
|
||
Unlike BYOND lists, ODLists also implement the "Constant Collections" and "Collection Capacity API" defined in subsequent sections. | ||
|
||
Example usage: | ||
``` | ||
var/odlist/foo = odlist("a","b","c") | ||
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.
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 rationale here is that 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. Couldnt think of a better name; but that seems good albeit smidge weird. 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.
|
||
|
||
foo += "d" // Valid modification, just like a BYOND list | ||
|
||
foo["c"] = 5 // Compile-time error, ODLists cannot be associative | ||
|
||
world.log << foo.Join() // Prints "abcd" | ||
``` | ||
|
||
### Queue | ||
Similar to lists, except elements are first-in first-out. Most of the list API will remain, except for inserting and removing elements. These will use `queue.enqueue(element)` and `queue.dequeue()`. | ||
|
||
Example usage: | ||
``` | ||
var/queue/foo = queue("a","b","c") | ||
|
||
foo += "d" // Compile-time error, must use foo.enqueue("d") | ||
|
||
world.log << foo[2] // Prints "b", indexing is supported | ||
|
||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would this be implicitly done if you do
or would you be forced to do
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. I like it being a method on queue, although it could potentially be more 'DMy' if it was something like 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. 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. |
||
|
||
### Stack | ||
Similar to queues, except elements are last-in first-out. Most of the list API will remain, except for inserting and removing elements. These will use `stack.push(element)` and `stack.pop()`. | ||
|
||
Example usage: | ||
``` | ||
var/stack/foo = stack("a","b","c") | ||
|
||
foo += "d" // Compile-time error, must use foo.push("d") | ||
|
||
world.log << foo[2] // Prints "b", indexing is supported | ||
|
||
foo.push("d") | ||
world.log << foo.pop() // Removes "d" from `foo` and prints "d" | ||
``` | ||
|
||
A `stack.to_array()` helper will also be provided, and returns the stack's elements as a shallow-copied `array()`. | ||
|
||
## Constant Collections | ||
|
||
All OpenDream collections can now be declared as constant, which prevents any future modifications to the collection contents and will enable future internal optimizations. | ||
ike709 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
Example usage: | ||
``` | ||
var/const/array/foo = array("a","b","c") | ||
foo[1] = "z" // Compile-time error, cannot modify constant collection | ||
Comment on lines
+149
to
+150
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. I think this needs some explanation of how it is going to work with relation to casting.
If the above was the case, then how would the following work?
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)? |
||
``` | ||
|
||
Note that `var/const/list/L` is not valid in BYOND and usages of constant collections will require a wrapper macro. | ||
|
||
## Collection Capacity API | ||
|
||
Collections (other than the existing BYOND `list()` and the fixed-length `array()` types) will expose the C# `Capacity` property and `EnsureCapacity()` functionality. These operate similarly to `list.len`, except capacity refers to the number of elements the collection's internal data structure can hold without allocating more memory. Ergo a list could have 3 elements and a capacity of 5, and no memory allocation will occur until a 6th element is inserted. | ||
|
||
Accessing `list.capacity` (and similar for other collection types) will return the currect value of the C# `List.Capacity`. Setting it will set it in C#, which may runtime if the new capacity is smaller than the current length of the collection. | ||
|
||
Calling `list.ensure_capacity(size)` (and similar for other collection types) will call the C# method `List.EnsureCapacity(size)`. This method ensures that the capacity of this collection is at least the specified capacity. If the current capacity is less than capacity, it is increased to at least the specified capacity. | ||
ike709 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
## BYOND List Deprecation Pragma | ||
|
||
A new `DeprecatedByondList` pragma will be implemented. This emits on any usage of BYOND `list()` collections, and will be disabled by default. This pragma exists to assist with guiding migration to OpenDream collections. | ||
|
||
## Considerations & Drawbacks | ||
|
||
The primary drawback is that these collections types provide no performance benefit to codebases that do not fully migrate to OpenDream, and maintaining BYOND compability relies heavily on `#ifdef OPENDREAM` wrapper macros. That being said, there is still the opportunity for OpenDream to catch misuse of these collections as part of CI workflows. |
This comment was marked as outdated.
Sorry, something went wrong.