[hub] Andrzej's review

Hi Everyone,
This is a short review of Boost.Hub.
First, I wanted to yet again thank Joaquín for this contribution. Also,
thanks to Ion for managing the review.

I personally never had a need for a container with colony/hive/hub
properties, so I do not speak as an expert in the problem domain. I have
read the docs for the library, investigated the API, compared against
std::hive, and played with the library on Compiler Explorer (GCC 15.2)
trying to break it.

I recommend accepting Hub into Boost. For me, it is equally fine whether
this would be a separate library or part of Boost.Container. The library is
clearly useful. It belongs in Boost more than in the Standard Library.

Regarding the design, there is not much flexibility here: it has already
been designed while standardizing std::hive. Although the STD has this
impractical type requirements Container and SequenceContainer that (i) are
not useful and (ii) do not play well with std::hive, so Boost.Hub has a
chance to set things straight. For instance, neither hub nor hive are
SequenceContainers, as pointed out in another thread.

As indicated in another thread, I recommend making the `visit` functions
free functions and renaming them to `for_each` and `for_each_while` (as per
Joaquí's suggestion). While the latter name is funny, it is the most
adequate one also. This could also be a customization point for all
containers: it would default to regular `std::for_each` but offer container
authors to provide a faster implementation.

We are encouraged to prefer Boost's Hub over Hive based on numbers, but it
is not clear to me if the better numbers are due to the necessary API
changes, or because Joaquín is a better implementer. What was the primary
motivation to scrap the block size control?

Regarding the documentation, I find the one currently present in the README
satisfactory (modulo the SequenceContainer requirements), and friendlier
than these in Boost.Container. So I wonder what the effect of merging would
be.

Regarding the implementation, I didn't have time to follow all of it but:
1. It is impressive that it is only 1800 lines.
2. It is easy to read and understand; also well documented, where needed.
3. I am concerned about the usage of  BOOST_LIKELY. Are these the source of
performance? It is my understanding that they are very fragile and may give
opposite effects once something changes in the compiler implementation.
Plus it is easy to fabricate the data that will make the program go slow
when the "negative" branches are deliberately triggered.
4. There is this line:
`size_type memory() const noexcept // TODO: remove`
Is this something to be fixed?
5. Compiling with -Wall gives me a warning in GCC when sorting an empty
hub: a zero-sized heap array is allocated:
https://godbolt.org/z/dWvxv9n8s
This is technically fine to allocate such arrays, but maybe it is worth to
make -Wall clean.

I know that Joaquín is responsive and am confident that the Hub
implementation will be maintained.

Regards,
&rzej;
_______________________________________________
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/ZTL2D2UMIHAAC6ZLHLH4Y3XETTQXAHBZ/