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

Subject: [boost] [SPAM] Re: [review] The review of Boost.DoubleEnded starts today: September 21 - September 30
From: Louis Dionne (ldionne.2_at_[hidden])
Date: 2017-10-06 23:54:35

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