Re: [review] The review of boost::container::hub starts! April 16 - April 26

El 24/04/2026 a las 15:47, Arnaud Becheler via Boost escribió:
> Dear Boost Community,
>
> This is my review for boost::container::hub.
>
> Thank you, Joaquin, for authoring such a high-quality proposal. Thank you,
> Ion, for managing the review.

Thanks for your review!

> I suggest: ACCEPT.
>
> The component is clearly delimited, and its equivalences and divergences
> with std::hive are clearly explained. I am impressed it's C++11 compatible,
> meaning anyone wanting to use std::hive but stuck with C++ < 26 will be
> very happy to find it. As usual, Joaquin developed extremely detailed
> benchmarks. The documentation is very good, although some formatting may
> need revisiting (unless Markdown is not the final destination format, in
> that case, please discard my comment. Edit: you will discard my comment :p
> ).
Yes, the case with documentation was somewhat unusual because the 
proposal is for integration in Boost.Container, so if the lib is 
accepted I don't expect the docs to survive in MD form.
> Given history, I trust Joaquin with the long-term management of the code.
>
>     - What is your evaluation of the design?
>
> Clearly reflects the std::hive design and documents any deviations from it.
>
>     - What is your evaluation of the implementation?
>
> Although I scrolled through the code, I am not knowledgeable enough about
> the domain to formulate relevant criticisms. But the code is clearly
> written, properly structured, and well-formatted (although the two-line
> lambdas looked weird to me aha). I saw a pair of comments but I wonder if
> they will be enough for future maintainers to understand the current
> implementation design. Although it's relatively short (1,880 lines), it may
> be easy to jump in if necessary.
>
>     - What is your evaluation of the documentation? (Note that, if
> accepted, final documentation will be included in Boost.Container docs,
> so please review the content, not the format)
>
> Oops. I am such a bad student for not reading through the entire exercise
> before diving in. I will leave my formatting remarks anyway. The
> documentation is excellent, I mention a few typos later in my review.
>
>     - What is your evaluation of its potential usefulness?
>
> This is obviously very useful.
>
>     - Did you try to use the container? With which compiler(s)? Did you
>       have any problems?
>
> I compiled a fabricated example with Apple Clang. No issue.
>
>     - How much effort did you put into your evaluation? A glance? A quick
>       reading? In-depth study?
>
> I spent ~5 hours on this evaluation. The documentation review was in-depth,
> but the code review was more of a glance.
>
>     - Are you knowledgeable about the problem domain?
>
> Not really, but having some knowledge of entity simulation, boost::hub may
> be very useful for my future explorations.
>
> Here are some random notes:
>
> - I like the example first documentation. It makes it immediately clear
> what the intent/API is.
> - Good explanation against std::list and its tradeoffs.
> - this line sounded weirdly complicated to me, i think it means that adding
> elements does not reallocate existing elements : > Pointers and iterators
> to an element remain valid as long as the element is not erased. hub will
> not reallocate elements as it grows in size.
Yes, your reading is correct, I'll try to rewrite the statement in a 
less convoluted way.
> - the trim_capacity example is not as clear as it could be; it deserves
> more explanation to be crystal clear.
Noted, will try to improve that part.
> - in the Motivation for a novel data structure: it introduces the design of
> a competitor, then presents the new design. It is not immediately clear
> what the tradeoffs are or if boost:hub design is better and why. It's like
> a proper section comparing competitors/current implementation is missing.
This is provided in 
https://github.com/joaquintides/hub#deviations-from-stdhive . Is this 
not sufficiently clear?
> - when comparing implementations, I tend to prefer comparative tables
> rather than paragraphs.
> - ascii tables are probably generated from standard output, so maybe it
> would not be too hard to automatically format them as markdown tables
> instead ? They would render in github, but I'm not sure about
> Boost.Container (edit: forget this formatting remark)
> - maybe use horizontal lines in the reference to distinguish each function
> block more clearly? (edit: forget this formatting remark)
Others have also complained about tables being clumsy in their current 
format, will try to improve on that. The reason I didn't produce MD 
tables is they end up being too wide.
> - the get_iterator section will make little sense for people who did not
> encounter such situation and wonder "why would we ever want this?".
> Lowering the bar for new boost users, it could maybe
> 1. start with a real case usage, e.g. in a legacy physics simulation I need
> to store T* in a map pointing into the hub. So when it's time to remove a
> particle, I have only the pointer, not the iterator that is required by
> erase()
> 2. at this point the naive user will wonder why not simply have an overload
> erase(T*) so it may be time to mention that it mimicks std::hive for the
> reason that STL convention is to not hide non-constant time operation
> behind constant time looking interfaces (if i understood correctly,
> otherwise please ignore).
> 3. Given that iterators are heavier, if you need to store millions of
> particles it's nice to just have a "ugly" one liner that allows to cut
> memory usage by 2
> 4. in terms of encapsulation it may make sense to hide the hub iterator
> type using T*
I have to confess I don't know myself what the potential usefulness of 
get_iterator is: the function is there because std::hive provides it. 
The rationale I give in the documentation is my bona fide speculation.
> - typos:
> // erase the element (couldn't be done *drectly* with p)
> // transfer non-empty blocks from h2 to h1 (no *rellocation*)
Already fixed: https://github.com/joaquintides/hub/pull/10/changes
>> Preconditions: *Tp* is DefaultInsertable into hub.
> missing new line: If the second overload is called, uses *alloc.
> Complexity: *Linear in x.size().
Good catches, will fix.
> Affiliation: C++ Alliance
>
> Thanks again to all of you for this very interesting piece of work.
> Throughout the review I kept picturing my old Windows 95 defragmenter
> running in the back of my mind, it was quite a refreshing remembrance ;)

Thank you 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/4JMWTQNVGB2X7HJWY3AK5Q63X3WB3CHV/