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