-
-
Notifications
You must be signed in to change notification settings - Fork 614
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
Rename Small_Array -> Flexible_Array #4251
base: master
Are you sure you want to change the base?
Conversation
core/container/array/array.odin
Outdated
@@ -0,0 +1,189 @@ | |||
/* | |||
This is an array that uses a fixed array as backing. The capacity of the array |
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.
I'd move the documentation of what it does over to the type. Also I don't like the wording usage of "this is X", because it may risk being unspecific.
Though, saying "An Array is an array that does [...]" also sounds strange, because it conflates with the builtin array type. I'm afraid that that is a seed for potential communication issues. Which is maybe why I would advocate for a different name, other than an Array.
I agree that the previous name, Small_Array
was weird. There's nothing in it to make it small, or at least smaller than a normal array. I also don't like the options you listed as alternatives in your PR description.
I have a few more suggestions, Growing_Array
, or maybe something that suggests a list, like Fixed_List
. Right now my suggestions aren't good, I'd have to look more into existing naming schemes. Also to note, the way this container functions is almost exactly like a dynamic array, equipped with nil
allocator. Maybe something to think about
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.
I welcome other name suggestions.
The weird problem with naming this thing is that I always feel like the name either has possible communication issues or that I disregard looking into the package because the name sounds weird.
I think at least the simple name Array
makes me more likely to open and read the docs instead of disregarding it like I did when it had a strange name. And if I open it and read the docs and see that it does not do what I want from an array, then I can look elsewhere.
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.
Btw, regarding this:
Also to note, the way this container functions is almost exactly like a dynamic array, equipped with nil allocator. Maybe something to think about
I think the Small_Array style of array is popular with the never-do-dynamic-allocations-crowd. I guess one could maybe setup the dynamic array to point to fixed memory though.
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.
I welcome other name suggestions.
Inline_Array
, in that the data is inline in the struct, not a pointer to the data.
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.
I welcome other name suggestions.
I don't like calling it just Array
; that's a significant name overlap with the concept of arrays. My first thought went to Limited_Array
, but all arrays are technically limited if they're not dynamic. Stack_Array
(stack as in heap/stack, not the datatype) has the potential to be confused with the idea of stack datatypes (and the struct could be allocated, too), but it does line up well with some of the API (push_back
, pop_front
).
Inline_Array
might be the better option.
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 convention I'm used to for this type of array, one that grows within its memory budget, is Fixed_Array
. I do like Inline_Array
though as that makes it easy to intuit where the storage for the array is.
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.
Inline_Array
sounds good to me. I set this PR as draft and will change to that name when I get some time.
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.
@flysand7 I kept the documentation at the top of the file. It feels more like an overview of both the type but also what the procedures in there do. However, I'm not sure if there is some official policy here, and I can change it if there is. Personally I really like overview comments at the top of files.
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.
Since its in its own separate package I guess either way would work fine. But generally speaking, I put documentation into the place where it applies the most. If it describes the functionality of the package as a whole, like why this package is here and what kinds of concepts we'll be using to document this packages' functions, it belongs to the package documentation. Otherwise it belongs to the type.
For example, with core:mem
docs I put the general concepts like pointer alignment, memory allocation, ZII into the package docs, because I will use these concepts and I felt it would be needed to be described somewhere that's not "under every single function in the package". I've also had trouble deciding whether the Arena
type is the one I'm gonna put the arena allocators' documentation to, or is it going to be the arena_allocator()
function. At the end I decided on the latter.
But that was a package with more than one thing in it, so, I don't know. It would be fine either way, but we should decide on our measure of consistency for this, because we can't have packages spread out in how they document themselves while implementing only one concept.
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.
That make sense. I've written big systems for game engines in the past, in those cases I really like a big overview comment at the top of the most central file. It's often the only documentation I need to read and it's the first thing I see.
I do not like the name I used
|
Normal arrays are also stored "inline", on the stack. Which doesn't really highlight a difference. Oh I guess dynamic array stores a slice. So then my contendant for the most descriptive name is: Okay that last part was a joke |
@flysand7 Well, the normal arrays are referred to as "fixed arrays" usually, so since users expect those to be "inline" then |
Dynamic arrays aren't inline. If you put "dynamic" into the name you no longer compare them to standard arrays, you're comparing them to dynamic arrays. |
The name |
@flysand7 I think now that perhaps having Dynamic in the name sends the wrong signals in general, regardless of placement, due to the connection to dynamic allocations.
|
I like flexible array! That might be the winner. |
|
c73380f
to
462e19c
Compare
Ready for review. I re-did this and used the name Motivation for the name |
Could you change the "usage example:" to "Example:" and indent the code with a tab so it generates nice docs like the other packages. |
@laytan Good idea, done! |
This renames
Small_Array
->Flexible_Array
and also adds some documentation at the top offlexible_array.odin
Rationale
Small_Array
is a strange name. TheSmall_Array
doesn't have to be small at all, you can have giantSmall_Array
s, just perhaps not as stack variables. But just like you can have giant global variables that contain fixed arrays, you can have giantSmall_Array
s as global variables.Motivation for the new name
Flexible_Array
: It doesn't use dynamic memory so I don't want the word dynamic in there. But also it is a fixed array under the surface, but it behaves in a kind-of semi-fixed way. So therefore it is "flexible".