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: Thorsten Ottosen (tottosen_at_[hidden])
Date: 2017-10-10 08:28:12

Hi Louis,

Thanks for the review. I forgot to give some feedback. Comments below.
> Hi,
> - 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.

Did you see the benchmarks?

Did you know Boost.Container has similar constructs? Would you then now 
not use Boost.Container?

>>     - What is your evaluation of the 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).

Can you explain why ? ... it is not obvious that devector differs in 
this respect.

> 
> - A simple diagram would be very helpful in quickly understanding what
>    the `devector` is.

Agreed.

> - 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`.

Some have been posted. What is your take on them?

> - 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.

I'm sure he was. Notice that reserve functions in batch_queue are not 
only to satisfy the precondition of unchecked_push_back/front, but also 
give the possibility to have a subsequent section of code that does not 
throw.

> 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.

Yes, I'm certainly biased. I don't know what is best: to have a review 
manager that doesn't care about the proposed library or one that does?

That said, we had GSOC in 2015 and should not do that if no one is 
willing to review the good end results. So Benedek asked for a review a 
long time ago, and no one came forward as review manager. A year or so 
later, I felt I had to do something about that. That's the background.

> 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.

Thanks.

-Thorsten