serge-sans-paille / frozen

a header-only, constexpr alternative to gperf for C++14 users
Apache License 2.0
1.31k stars 104 forks source link

Added additional functionality to cvector and exposed public array.h and vector.h. #123

Open adamshapiro0 opened 3 years ago

serge-sans-paille commented 3 years ago

Can you share the motivation behind this commit? If you want to make them public, you need to provide extensive testing wrt. to standard library API, just like it's done for other types. And add a word about it in the README.

adamshapiro0 commented 3 years ago

I think having access to constexpr-capable vector and array similar to the other STL-like containers would be quite useful for a lot of applications. In our use case, for example, we're defining a system configuration structure that needs to work on high-level platforms where we can dynamically change parameters via a runtime config file (JSON, etc.), and also on embedded platforms where the config needs to be built into .rodata in flash. We have some parameters that take a variable number of values, i.e., vector. For instance, one parameter is an array of progressively longer reset intervals ("reset after 5 seconds, then if that doesn't work reset after 20, etc."), which we define as frozen::vector<float, 3> reset_interval_sec{5.0f, 20.0f}; but might be set differently or overridden depending on the platform.

I had to modify the class in order to use vector with an initializer list. Once I did that I noticed that it only had a small subset of the STL function definitions, unlike carray, so I added the rest in case they're useful for anyone down the road. Then I figured it made sense to make them public since they're really useful and including internal stuff from bits is not really good form.

I'm happy to add some unit tests when I get a little time to do so. Figured I'd push this change up as is to get the discussion started. Do you agree with having using statements to alias the types from frozen::bits, or do you think the class definitions should be moved out of basic_types.h and any classes that use them like set instead include the top-level headers? Any concerns?

adamshapiro0 commented 3 years ago

Hey @serge-sans-paille, just following up. I added unit tests a couple weeks ago as requested. I am also currently adding erase() and insert() support, but I can add that in a separate PR if you prefer. Just let me know.

Just to add another use case: we're currently porting some code from a more powerful platform with lots of memory to a very small microcontroller. The .rodata configuration case above is for that effort. In addition to that, we're leveraging frozen::vector to try to eliminate lots of heap allocations in places where the code currently uses STL containers to avoid memory fragmentation. In that case, we do not want the vector to be constexpr, and ideally want it to be as close as possible to a drop-in replacement for std::vector, just with a known fixed size to avoid the heap. Hence adding erase() and insert().

For some of these replacements, ideally we would want to use frozen::set and frozen::map, but unfortunately those are currently sorted at compile time, so can't support modification at runtime. To get around that for now, we're sorting the frozen::vector and then implementing contains() and find() behavior using std::binary_search() and std::lower_bound().

Even for constexpr use like in our .rodata configuration case, set and map also don't support initialization with a list of items smaller than the max capacity. I tried to figure out how to modify them to use cvector instead of carray but couldn't quite get the PMH details right. Seems like you might have run into similar issues in this PR: https://github.com/serge-sans-paille/frozen/pull/113?

serge-sans-paille commented 3 years ago

Hey @adamshapiro0 I've spent much time on frozen recently, sorry for the delay, and thanks for both the hard work and the motivation sharing.

I'm fine with the idea of exposing frozen::array in the public API, it's roughly a C++14 implementation of C++17's std::array.

I'm not as fine with the idea of exposing cvector, which doesn't map to an existing standard container (granted it looks like a std::vector, but it's not in many ways).

I still value your work, so here is a plan : split that PR in two, one for array + exposing it in the public API, one for vector and keep it in its original namespace.

Would you be ok with that?

adamshapiro0 commented 3 years ago

Well to be clear, we are using frozen::vector (i.e., cvector) to accomplish the 2 use cases I described above, so not exposing it doesn't work for us unfortunately. I added frozen::array for consistency, but we don't really use it for anything.

Can you explain what you mean about it not mapping to an existing container? As far as I can tell it works exactly like a std::vector, just with a fixed capacity. Certainly that was the intent at least, and it's how we're using it in our code to A) allow for constexpr definitions of some lists of data in .rodata space, and B) eliminate heap usage in other parts of the code where we still need a dynamic container at runtime. It's extremely useful for both of those use cases.

The only exceptions to the std::vector functionality that I can think of are of some of the move operators and emplace_back(), which are missing but can definitely be added. If anything is missing or incorrect, I'm happy to make those changes.