Re: [boost] [SPAM] Re: [review] The review of Boost.DoubleEnded starts today: September 21 - September 30

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 <
boost_at_[hidden]> 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