Hi, This is my review of the proposed Boost.DoubleEnded library. I did my review and read the threads on the ML only after (to avoid bias), so I know some of the points below have already been raised by other reviewers. I vote to REJECT the library as currently proposed. However, I could see parts of the functionality provided by this library be added to Boost.Container.
- What is your evaluation of the design?
Major points: - The first thing I thought when I saw the library was: why is this not an addition to Boost.Container? - Regarding `batch_deque`, is there any reason why it can't simply be unified with `boost::container::deque` by adding more customization points to the latter? - I don't see the use for the uninitialized constructors and `unsafe_push_back`/`unsafe_push_front`. I mean, I do understand the use _in theory_, but I'd really like to see a good motivation for them with a more concrete use case. This is pretty dangerous and tricky to work with, so I think this deserves a good justification. I can't see myself using this container or recommending anyone to use the container without this. Minor points: - `de::reserve_only_tag{}` could be just `de::reserve_only` by providing the following definition: static constexpr de::reserve_only_tag reserve_only{}; - `de::batch_deque_policy<256>` should really be `de::segment_size<256>` or something like that.
- What is your evaluation of the implementation?
Looks okay with a quick study. However, I found a few red flags that made me uneasy. For example, https://git.io/vdu3x: // TODO should move elems-by-elems if the two allocators differ Given the fact that it's a container library with support for custom allocators, this makes me wonder whether there are other oversights like this and whether the library is Boost quality as of now. I'm not sure, as I may be misunderstanding what the TODO means. Also, the fact that the library has not seen significant work in 1.5 years makes me think that this TODO might not go away.
- What is your evaluation of the documentation?
Not bad, but it could use some more work. - I found it quite difficult to get something significant out of the tutorial documentation without having read the reference documentation before. This is because the documentation makes several references to particularities of the containers that are only explained in the reference documentation. - The design rationale for `devector` at the beginning of the documentation is slightly misleading. `devector` can't be a drop-in replacement for `std::vector`, because of iterator invalidation guarantees of move operations (see https://stackoverflow.com/a/41384937/627587). - In section "devector usage": > Here, while push_back calls make use of the small buffer, push_front > results in an allocation: Push operations do not shift elements to > avoid breaking the promise of reserve. The "promise of reserve" needs to be explained; what does this mean? - How do those containers play with custom Allocators (I'm specifically thinking about those that provide fancy pointers)? The documentation should mention this. - A simple diagram would be very helpful in quickly understanding what the `devector` is. - I would really like to see actual benchmarks, even if this needs coming up with more realistic use cases. IMO, it is not acceptable for a library tailored for performance (and having unsafe operations for that purpose) to provide only a listing of assembly at `-O2`. - Several minor things like typos and weird formulations. I submitted a PR for some of those, but the situation could still be improved. This is not a big deal, though, as it would be trivial to fix. - I must say I appreciate the attention that was given to exception guarantees in the documentation. This is quite important for containers, and it seems like the author was careful about this.
- What is your evaluation of the potential usefulness of the library?
This is where it falls apart for me. I feel like the documentation fails to make a case for the library, and as a result, I don't see myself using it or recommending someone else to use it. I do understand the particularities of the provided containers, I just fail to see why I should care enough about them to justify a Boost library. As I said before, I also think the unsafe operations should be better justified, because otherwise one of the purposes of the library seems to be to aid C++ programmers to shoot themselves in the foot.
- Did you try to use the library? With which compiler(s)? Did you have any problems?
I built the unit tests on my Mac with Clang without problem. I did have problems when I tried building without specifying a toolset, but this may be a misunderstanding of how Boost.Build works on my part.
- How much effort did you put into your evaluation? A glance? A quick reading? In-depth study?
About 3 hours of reading all the documentation and looking at the implementation.
- Are you knowledgeable about the problem domain?
I know stuff about containers, but I'm not an expert. I'm not exactly sure what the "problem domain" of the library is, which is one of the issues I have with it. Also, I'd like to point out that formal reviews are usually carried out by review managers without direct involvement in the library. I do not want to imply that the review outcome will actually be biased, and I'm sure the review manager is capable of impartiality, but I thought this was worth pointing out. Like I said at the beginning, I don't think this should be a new Boost library. However, I think it would be worthwhile to consider adding `devector` to Boost.Container and augmenting Boost.Container's `deque`, and I would encourage the author of the library to pursue this path. Thank you, Louis Dionne -- Sent from: http://boost.2283326.n4.nabble.com/Boost-Dev-f2600599.html