Subject: Re: [boost] [SPAM] Re: [review] The review of Boost.DoubleEnded starts today: September 21 - September 30
From: Benedek Thaler (thalerbenedek_at_[hidden])
Date: 2017-10-07 10:35:31
On Sat, Oct 7, 2017 at 1:54 AM, Louis Dionne via Boost <
> 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.