Skip to content
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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

karl-zylinski
Copy link
Contributor

@karl-zylinski karl-zylinski commented Sep 16, 2024

This renames Small_Array -> Flexible_Array and also adds some documentation at the top of flexible_array.odin

Rationale

Small_Array is a strange name. The Small_Array doesn't have to be small at all, you can have giant Small_Arrays, just perhaps not as stack variables. But just like you can have giant global variables that contain fixed arrays, you can have giant Small_Arrays 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".

@@ -0,0 +1,189 @@
/*
This is an array that uses a fixed array as backing. The capacity of the array
Copy link
Contributor

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

Copy link
Contributor Author

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.

Copy link
Contributor Author

@karl-zylinski karl-zylinski Sep 16, 2024

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.

Copy link
Member

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.

Copy link
Contributor

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.

Copy link

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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.

@karl-zylinski karl-zylinski marked this pull request as draft September 17, 2024 07:09
@gingerBill
Copy link
Member

I do not like the name Array because that is way too overloaded of a term.

I used Small_Array because it's to suggest it is for "small" arrays 😛

Inline_Array would work but I'd argue still confusing as a term.

@flysand7
Copy link
Contributor

Fixed_Dynamic_Array sounds like the most descriptive name to me. It's not just an array, because it grows. It's also not just a dynamic array because the backing buffer is fixed and no allocator is 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: Fixed_Inline_Dynamic_Array (FIDA)

Okay that last part was a joke

@karl-zylinski
Copy link
Contributor Author

@flysand7 Well, the normal arrays are referred to as "fixed arrays" usually, so since users expect those to be "inline" then Fixed_Dynamic_Array should be understandable as well. I guess someone might argue that it is a bit wordy.

@flysand7
Copy link
Contributor

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.

@flysand7
Copy link
Contributor

The name Dynamic_Fixed_Array might get around that

@karl-zylinski
Copy link
Contributor Author

@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.

Growing_Fixed_Array
Flexible_Array -- it doesn't use dynamic memory and it's only "half fixed"... It's flexible!

@flysand7
Copy link
Contributor

I like flexible array! That might be the winner.

@Feoramund
Copy link
Contributor

Flexible_Array sounds like the better of all the offered options so far. The only other I could think of was Struct_Array which is probably an obtuse name.

@karl-zylinski karl-zylinski changed the title Rename Small_Array -> Array Rename Small_Array -> Flexible_Array Sep 19, 2024
@karl-zylinski karl-zylinski marked this pull request as ready for review September 19, 2024 20:07
@karl-zylinski
Copy link
Contributor Author

Ready for review. I re-did this and used the name Flexible_Array instead.

Motivation for the name Flexible_Array: This array 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".

@laytan
Copy link
Collaborator

laytan commented Sep 19, 2024

Could you change the "usage example:" to "Example:" and indent the code with a tab so it generates nice docs like the other packages.

@karl-zylinski
Copy link
Contributor Author

@laytan Good idea, done!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants