[review] Reminder: The review of boost::container::hub ends April 26
Dear all, A kind reminder, hub's review will end soon. Please consider taking a bit of time to review this interesting container ;-) The proposal is small enough anyone can review it and it might be useful for many people! It may be downloaded from https://github.com/joaquintides/hub and the documentation may be found here: https://github.com/joaquintides/hub/blob/develop/README.md boost::container::hub is a sequence container with O(1) insertion and erasure and element stability: pointers/iterators to an element remain valid as long as the element is not erased. boost::container::hub is very similar but not entirely equivalent to C++26 std::hive Waiting you reviews! Best, Ion (Review Manager)
On Fri, Apr 24, 2026 at 7:27 AM Ion Gaztañaga via Boost < boost@lists.boost.org> wrote:
Dear all,
A kind reminder, hub's review will end soon. Please consider taking a bit of time to review this interesting container ;-) The proposal is small enough anyone can review it and it might be useful for many people!
My vote: ACCEPT I like the the idea of using bit masks to track occupancy, trading flexibility for lower per-slot overhead compared to hive. The documentation specifies: "minimum and maximum block size limits can't be specified and are fixed to 64". I recommend to leave the block size unspecified (and remove all mentions of 64 from the documentation). The implementation is of very high quality, but there are a couple of issues: - In my reading the constructors will leak blocks if T's copy/move or the allocator throws midway, because in this situation ~hub() will not be called and block_list has no destructor. - sort_iterator::operator[] uses operator*(*this + n), which I think won't compile because it's invoking the free operator*. - transfer_sort allocates the temporary via buffer<T, Allocator> which allocates via operator new[](n * sizeof(T), std::nothrow), which is incorrect for over-aligned T. - the documentation for transfer_sort says that if an exception is thrown, the order of the elements is unspecified but in fact the resulting state could be worse than that -- the documentation should say "basic guarantee" and leave it at that. - get_iterator should assert if the user passes an invalid pointer. I'd assert on the mask bit being set. The biggest issue is the lack of exception-safety tests. Emil
El 26/04/2026 a las 23:28, Emil Dotchevski via Boost escribió:
On Fri, Apr 24, 2026 at 7:27 AM Ion Gaztañaga via Boost < boost@lists.boost.org> wrote:
Dear all,
A kind reminder, hub's review will end soon. Please consider taking a bit of time to review this interesting container ;-) The proposal is small enough anyone can review it and it might be useful for many people! Hi Emil, thanks for your review! My vote: ACCEPT
I like the the idea of using bit masks to track occupancy, trading flexibility for lower per-slot overhead compared to hive. The documentation specifies: "minimum and maximum block size limits can't be specified and are fixed to 64". I recommend to leave the block size unspecified (and remove all mentions of 64 from the documentation). Yes, I definitely should remove "64" from the reference section. As for the rest of the docs, I feel like I should mention it somehow, but, as you imply, it should be made in a noncomittal way. The implementation is of very high quality, but there are a couple of issues:
- In my reading the constructors will leak blocks if T's copy/move or the allocator throws midway, because in this situation ~hub() will not be called and block_list has no destructor.
I think this is taken care of: all non-noexcept hub constructors are _delegating_ constructors. For instance, this constructor template< typename InputIterator, typename = hub_detail::enable_if_is_input_iterator_t<InputIterator> > hub( InputIterator first, InputIterator last, const Allocator& al_ = Allocator()): hub{al_} { insert(first, last); } delegates to hub{al_}, so if an exception is thrown on insert(first, last), ~hub() will indeed be called.
- sort_iterator::operator[] uses operator*(*this + n), which I think won't compile because it's invoking the free operator*.
Thanks, Matt Borland spotted that too, fixed here: https://github.com/joaquintides/hub/pull/8 (Changes are being made to branch feature/review-feedback to keep develop stable during the review.)
- transfer_sort allocates the temporary via buffer<T, Allocator> which allocates via operator new[](n * sizeof(T), std::nothrow), which is incorrect for over-aligned T. You're right. For C++17 I can use a std::align_val_t variant of new[]. I understand I'm out of luck for C++14 and earlier, right? - the documentation for transfer_sort says that if an exception is thrown, the order of the elements is unspecified but in fact the resulting state could be worse than that -- the documentation should say "basic guarantee" and leave it at that.
Will fix. FWIW I copied what the standard says, but the standard itself should clarify this too. Rereading that part, the standard draft says that the preconditions for sort are: T is Cpp17MoveInsertable into hive, Cpp17MoveAssignable, and Cpp17Swappable. I think there's a missing precondition that T be Cpp17MoveConstructible, since the intent is that an implementation may do as transfer_sort does (at least, both plf::hive and the ongoing impl for MSVC STL (https://github.com/NylteJ/STL/tree/hive) do).
- get_iterator should assert if the user passes an invalid pointer. I'd assert on the mask bit being set. Yes. I'm a little mystified by the standard spec for this as get_iterator is marked noexcept, so it's violating Lakos Rule if I'm not wrong. DR worthy? The biggest issue is the lack of exception-safety tests.
I will add those tests. Thanks again for your time and feedback! Joaquín M López Muñoz
Joaquin M López Muñoz wrote:
- get_iterator should assert if the user passes an invalid pointer. I'd assert on the mask bit being set. Yes. I'm a little mystified by the standard spec for this as get_iterator is marked noexcept, so it's violating Lakos Rule if I'm not wrong. DR worthy?
Probably. It's not clear how Lakos interacts with hardened preconditions, but it's worth an open issue either way.
pon., 27 kwi 2026 o 16:08 Peter Dimov via Boost <boost@lists.boost.org> napisał(a):
Joaquin M López Muñoz wrote:
- get_iterator should assert if the user passes an invalid pointer. I'd assert on the mask bit being set. Yes. I'm a little mystified by the standard spec for this as get_iterator is marked noexcept, so it's violating Lakos Rule if I'm not wrong. DR worthy?
Probably.
It's not clear how Lakos interacts with hardened preconditions, but it's worth an open issue either way.
A hardened precondition is one that runtime-checks and aborts in a hardened implementation, and that behaves like any other precondition in non-hardened implementation. Switching between a hardened and a non-hardened implementation can be done via a compiler command flag. Because of this non-hardened variant, the Lakos rule still applies. Regards, &rzej;
_______________________________________________ Boost mailing list -- boost@lists.boost.org To unsubscribe send an email to boost-leave@lists.boost.org https://lists.boost.org/mailman3/lists/boost.lists.boost.org/ Archived at: https://lists.boost.org/archives/list/boost@lists.boost.org/message/DQXIOU5C...
El 26/04/2026 a las 23:28, Emil Dotchevski via Boost escribió:
On Fri, Apr 24, 2026 at 7:27 AM Ion Gaztañaga via Boost < boost@lists.boost.org> wrote:
Dear all,
A kind reminder, hub's review will end soon. Please consider taking a bit of time to review this interesting container ;-) The proposal is small enough anyone can review it and it might be useful for many people!
My vote: ACCEPT
Thanks for your review Emil, Ion
participants (5)
-
Andrzej Krzemienski -
Emil Dotchevski -
Ion Gaztañaga -
Joaquin M López Muñoz -
Peter Dimov