[review] [hub] Review result: ACCEPTED

Hi to all,

I'm glad to announce that the proposed boost::container::hub container 
has been ACCEPTED. Congrats Joaquín!

I'll try to summarize the comments/actions/changes/suggestions from 
reviewers. We have received 6 reviews:

- Andrzej Krzemienski: Accept — "I recommend accepting Hub into Boost."

- Amlal El Mahrouss: ACCEPT — merge into container.

- Arnaud Becheler: ACCEPT — "I suggest: ACCEPT."

- Christian Mazakas: ACCEPT — "strongly Accept" into Container.

- Matt Borland: ACCEPT — explicit closing vote.

- Emil Dotchevski: ACCEPT — "My vote: ACCEPT" (with a list of issues).

- Peter Turcan: (documentation comments; no explicit ballot line)

-------------------
Andrzej Krzemienski
-------------------

Praises:

- Easy to read and understand

- Well documented, where needed.

Main comments:

- Recommends making the `visit` functions free functions and renaming 
them to `for_each` and `for_each_while` --> Joaquín agrees

- std::hive and hub are not SequenceContainers, docs should make this 
explicit --> Joaquín agrees.

- Questions BOOST_LIKELY --> Joaquín clarified BOOST_LIKELY is not the 
core performance story.

- "memory()" is marked with "// TODO: remove;" --> Joaquín agrees to 
remove it.

- Reported a GCC -Wall warning when sorting an empty hub (Peter Dimov 
flagged it as a false positive and suggested a fix) --> Joaquín agrees 
to fix this.

-----------------
Amlal El Mahrouss
-----------------

Amlal praises several aspects of the library:

- Preconditions

- Usage of known requirements (like CopyInsertable)

- Free functions

- NATVIS for MSVC

- Implementation readability

- BOOST_CONTAINER_HUB_PREFETCH transparency

------------
Matt Borland
------------
Points to:

- Strong design decisions and clear deviations from std::hive

- Usefulness is clear given std::hive in C++26; on testing

- Clean implementation

- Appreciates pretty printers

Suggestions:

- Stricter warnings (-Wall, -Werror, -Wold-style-cast) --> Joaquín will 
tighten the warnings

- Questions MSVC minimum (14.3) vs very old GCC/Clang --> Joaquín will 
try to handle that with the help of Boost.CI

- Finds ASCII performance tables hard to read, suggests implementing 
Peter Turcan-style improvements.

- Reports 4 of 5 tests failed on ARM Mac with Clang 22 / GCC 15 --> PRs 
reported, one liner fix --> Joaquín will fix it and will improve the 
regression with the help of the latest Boost.CI improvements.

---------------	
Arnaud Becheler
---------------
Praises:

- C++11 portability

- Benchmarks

- Code looks clear

- Docs

Comments about docs that Joaquín will address:

- trim_capacity example needs clarity

- "Pointers and iterators to an element" section needs improvement

- motivation could use a clearer competitor comparison

- prefers comparative tables; ASCII tables are awkward (Joaquín 
clarifies final docs will not stay Markdown)

- get_iterator needs a motivating story for users who only have T* 
(Joaquín admits uncertainty on real-world value of get_iterator beyond 
standard API parity)

- Typos and line-break nits

-----------------
Christian Mazakas
-----------------

- General agreeement.

- Minor caution that insertion order is not preserved when slots are 
reused, should be stated in the docs

Christian also suggets a future extension for slotmap-style alternative. 
Joaquín agrees to review this suggestion in the future and also points 
to a proposal for a "reserving" (virtual memory) allocator idea as 
future exploration.


-----------------
Emil Dotchevski
-----------------

Likes:

- The bitmask approach vs hive.

- Very high quality implementation


Notes/suggestions:

- We should leave block size unspecified in documentation and removing 
explicit "64" in docs.

- Possible leaks in constructors if block_list lacks cleanup (Joaquín 
clarified it's not the case)

- sort_iterator::operator[] and operator* --> Joaquín will fix it

- transfer_sort buffer and over-alignment --> over-aligned new[] will be 
used where available.

- exception guarantee text for transfer_sort should say basic guarantee

- get_iterator should assert validity

Joaquín also said that he will add exception-safety tests.

------------
Peter Turcan
------------

Peter focused on clarity and adoption improvements:

- Opening sentence and HFT acronym (should be clarified) --> Joaquín 
will revise intro

- Deeper, less conversational use-case treatment

- Explain the role of 64 and why it is not freely tunable --> Joaquín 
agrees (also comments 128 would be nicer if hardware had a single 
instruction)

- Prefer plain-English comments

- Clarify trim_capacity / reserve comments --> OK

- Avoid excessive brackets on the performance section and clarify some 
numbers there --> OK

- Reference could use more exception / example cross-links --> OK

------------
------------

Joaquín is already working on changes based on the review feedback here:

https://github.com/joaquintides/hub/tree/feature/review-feedback

I will work with Joaquín to find the best way to integrate 
code/docs/tests into Boost.Container.

Best,

Ion
_______________________________________________
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/7WZ7QTPE2YDYD5OYCKXKKV2N74JHJRZL/