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