On Sat, Oct 7, 2017 at 1:54 AM, Louis Dionne via Boost < boost@lists.boost.org> wrote:
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.
Thanks for your review! (And the PR fixing typos)
- 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?
I suppose this is now explained above. - 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.
Again, benchmarks above - they show some things are faster this way. On the other hand, I absolutely get your point.
- 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).
I suppose this happens only with a small buffer in effect. - 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?
d.reserve(d.size() + 10); // next 10 push_back will not allocate
- How do those containers play with custom Allocators (I'm specifically thinking about those that provide fancy pointers)? The documentation should mention this.
Probably not supported. https://github.com/erenon/double_ended/issues/2
- 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`.
See above - tough I'm skeptic regarding micro-benchmarks. Too easy to get them wrong, without knowing it, no way to prove one got them right.
- 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.
Thanks, merged. Benedek