Generic container wrapper type with a default underlying buffer (2nd rev)

Question

This is a revision of the previous question. I was told to ask an additional question with more context for the modified code to be reviewed.

The changes:

1. changed from struct to class (for semantic purposes) and made the member variables private;
2. added a public member function get_container to allow access to the only data member that the user code needs;
3. added a private type alias value_type.

My general aim is to keep things as simple as appropriate. This means I prefer not to use some features (e.g. inheritance, CRTP, etc.) and this may lead to slightly less convenient API which is fine (in my opinion).

Here is the modified code:

template <class Container, std::size_t DefaultBufferCapacity> requires ( std::same_as<typename Container::allocator_type, std::pmr::polymorphic_allocator<typename Container::value_type>> ) class [[ nodiscard ]] buffer_resource_container { public: buffer_resource_container( ) noexcept ( noexcept( Container { &buffer_resource } ) ) = default; auto& get_container( this auto& self ) noexcept { return self.container; } private: using value_type = Container::value_type; alignas ( value_type ) std::array<std::byte, DefaultBufferCapacity * sizeof( value_type )> buffer; std::pmr::monotonic_buffer_resource buffer_resource { std::data( buffer ), std::size( buffer ) }; Container container { &buffer_resource }; }; template <class ValueType, std::size_t DefaultBufferCapacity> using buffer_resource_vector = buffer_resource_container<std::pmr::vector<ValueType>, DefaultBufferCapacity>; template <class ValueType, class Allocator> [[ nodiscard ]] std::errc inline reserve_capacity( std::vector<ValueType, Allocator>& vec, const std::size_t new_capacity ) noexcept { try { vec.reserve( new_capacity ); } catch ( const std::bad_alloc& ) { return std::errc::not_enough_memory; } catch ( const std::length_error& ) { return std::errc::value_too_large; } catch ( const std::exception& ) { return std::errc::resource_unavailable_try_again; } return std::errc { }; } 

A good demonstrative case in which I actually use the above code is shown below:

#include <concepts> #include <memory_resource> #include <array> #include <vector> #include <system_error> #include <new> #include <stdexcept> #include <exception> #include <chrono> #include <expected> #include <span> #include <string_view> #include <iterator> #include <algorithm> #include <iostream> #include <cstddef> template <class Container, std::size_t DefaultBufferCapacity> requires ( std::same_as<typename Container::allocator_type, std::pmr::polymorphic_allocator<typename Container::value_type>> ) class [[ nodiscard ]] buffer_resource_container { public: buffer_resource_container( ) noexcept ( noexcept( Container { &buffer_resource } ) ) = default; auto& get_container( this auto& self ) noexcept { return self.container; } private: using value_type = Container::value_type; alignas ( value_type ) std::array<std::byte, DefaultBufferCapacity * sizeof( value_type )> buffer; std::pmr::monotonic_buffer_resource buffer_resource { std::data( buffer ), std::size( buffer ) }; Container container { &buffer_resource }; }; template <class ValueType, std::size_t DefaultBufferCapacity> using buffer_resource_vector = buffer_resource_container<std::pmr::vector<ValueType>, DefaultBufferCapacity>; template <class ValueType, class Allocator> [[ nodiscard ]] std::errc inline reserve_capacity( std::vector<ValueType, Allocator>& vec, const std::size_t new_capacity ) noexcept { try { vec.reserve( new_capacity ); } catch ( const std::bad_alloc& ) { return std::errc::not_enough_memory; } catch ( const std::length_error& ) { return std::errc::value_too_large; } catch ( const std::exception& ) { return std::errc::resource_unavailable_try_again; } return std::errc { }; } namespace { [[ nodiscard ]] bool try_initialize_timezone_database( ) noexcept { bool is_initialized { }; try { [[ maybe_unused ]] const auto& tzdb_list { std::chrono::get_tzdb_list( ) }; is_initialized = true; } catch ( const std::runtime_error& ) { } return is_initialized; } constexpr auto timezone_names_default_count { 1000uz }; [[ nodiscard ]] std::expected<const std::span<const std::string_view>, std::errc> retrieve_all_timezone_names( const bool is_tzdb_initialized ) noexcept { if ( is_tzdb_initialized == false ) return std::unexpected { std::errc::state_not_recoverable }; const auto& tzdb { std::chrono::get_tzdb( ) }; const auto timezone_names_count { tzdb.zones.size( ) + tzdb.links.size( ) }; static buffer_resource_vector<std::string_view, timezone_names_default_count> brv { }; auto& timezone_names_buffer { brv.get_container( ) }; const auto err_code { reserve_capacity( timezone_names_buffer, timezone_names_count ) }; if ( err_code != std::errc { } ) return std::unexpected { err_code }; const auto populate_timezone_names_buffer { [ &tzdb, inserter = std::back_inserter( timezone_names_buffer ) ]( ) noexcept { std::ranges::transform( tzdb.zones, inserter, &std::chrono::time_zone::name ); std::ranges::transform( tzdb.links, inserter, &std::chrono::time_zone_link::name ); } }; populate_timezone_names_buffer( ); return timezone_names_buffer; } } const auto is_tzdb_initialized { try_initialize_timezone_database( ) }; const auto all_timezone_names { retrieve_all_timezone_names( is_tzdb_initialized ) }; int main( ) { if ( is_tzdb_initialized == false ) { std::cout << "The timezone database could not be initialized\n"; return 1; } if ( all_timezone_names.has_value( ) == false ) { std::cout << "An error occurred during buffer allocation\n"; return 1; } const auto timezone_names { *all_timezone_names }; } 

Looking at the above demo program, I think that this container wrapper is a good solution for storing a limited number of std::string_views without doing a dynamic allocation and still having a fallback to dynamic allocation if the number of timezone names exceeds the default count (i.e. 1000) in the future.

---

Any suggestions for improvements? Or any potential downsides?

Answer 1

About API convenience

[…] I prefer not to use some features (e.g. inheritance, CRTP, etc.) and this may lead to slightly less convenient API which is fine (in my opinion).

Making your buffer_resource_container appear like a regular STL container is not just convenience, it also makes it possible to use your class as a drop-in replacement for existing containers types. Consider this code:

std::vector<int> foo; foo.push_back(42); std::sort(foo.begin(), foo.end()); … 

Ideally, I'd only have to change the first line to:

buffer_resource_vector<int, 10> foo; 

But with your code I also have to change any line that actually uses foo:

foo.get_container().push_back(42); std::sort(foo.get_container().begin(), foo.get_container().end()); … 

Sure, you can store the result of get_container() in a reference and use that, which could avoid some of the changes, but with a drop-in replacement it wouldn't be necessary at all.

However, if it is acceptable for you to just have a get_container() member function, then that is fine.

Is it suitable for your use case?

I think your class certainly has some value. However, in the context of loading the timezone database, I don't think it is very useful. This is something you would only be doing once in your program, and avoiding one memory allocation is not a worthwhile performance optimization. The hardcoded default size of 1000 entries is very likely either too small, so it will require an allocation anyway, or too big, which wastes memory.

retrieve_all_timezone_names() is unsafe

This function is unsafe for several reasons. First, you can just call it with true as an argument, even if you didn't initialize the timezone database. Furthermore, it might be accidentally called twice. The first time it will work fine, but the second time it will add all timezone database entries again to the static brv. However, this might cause the std::pmr::vector in it to resize its allocation, causing it to move its elements in memory (whether that is still inside the monotonic buffer resource or not doesn't matter!). This invalidates the std::span returned by the first call to retrieve_all_timezone_names().

I recommend you just have one function to retrieve the timezone names, which is structured like so:

std::expected<const std::span<const std::string_view>, std::errc> retrieve_all_timezone_names() noexcept { static buffer_resource_vector<std::string_view, timezone_names_default_count> brv; static std::expected<const std::span<const std::string_view>, std::errc> timezone_names = [&]{ try { std::chrono::get_tzdb_list(); } except(const std::runtime_error&) { return std::unexpected{std::errc::state_not_recoverable}; } const auto& tzdb{std::chrono::get_tzdb()}; const auto timezone_names_count{tzdb.zones.size() + tzdb.links.size()}; auto& timezone_names_buffer{brv.get_container()}; const auto err_code{reserve_capacity(timezone_names_buffer, timezone_names_count)}; if (err_code != std::errc{}) return std::unexpected {err_code}; auto inserter = std::back_inserter(timezone_names_buffer); std::ranges::transform(tzdb.zones, inserter, &std::chrono::time_zone::name); std::ranges::transform(tzdb.links, inserter, &std::chrono::time_zone_link::name); return timezone_names_buffer; }(); return timezone_names; } 

This uses a static variable inside a function. Its initialization is guaranteed to be done atomically at the first function call (so it's thread-safe). To do this complex initialization an immediately invoked lambda expression is used. This prevents this function from being called in an unsafe way.