Re: [review] Reminder: The review of boost::container::hub ends April 26

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 <
> [email protected]> 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

_______________________________________________
Boost mailing list -- [email protected]
To unsubscribe send an email to [email protected]
https://lists.boost.org/mailman3/lists/boost.lists.boost.org/
Archived at: https://lists.boost.org/archives/list/[email protected]/message/7DADDDG7PRAZUAZDHCDLTVQBWIV5YRMY/