[libcxx-commits] [PATCH] D117108: [libc++] Remove vector base class

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone added inline comments.


================
Comment at: libcxx/include/vector:1048
 template <class _Tp, class _Allocator>
 vector<_Tp, _Allocator>::vector(size_type __n)
 {
----------------
ldionne wrote:
> I think it's weird that we're default initializing the allocator instead of value initializing it here, but it's definitely consistent with what we did before this patch.
> 
> Actually, this leads me to believe that we don't implement the C++20 `vector` properly. Indeed, in C++20, this constructor is specified as:
> 
> ```
> constexpr explicit vector(size_type n, const Allocator& = Allocator());
> ```
> 
> Unless mistake, this means that we should be value-initializing the allocator here to be equivalent to the default argument. This seems to have changed in C++14 (https://en.cppreference.com/w/cpp/container/vector/vector). @Quuxplusone, do you happen to know what paper changed this? Does this look suspicious to you too?
> 
> This comment doesn't affect this review since it's not a behavior change with this patch, but we should rectify the situation in a follow-up patch if my suspicion above is confirmed.
> This seems to have changed in C++14. @Quuxplusone, do you happen to know what paper changed this? Does this look suspicious to you too?

Whenever you see default arguments (which are the devil) and the shuffling-around of ctor overload sets, it's almost certainly due to [p1163](http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/p1163r0.pdf)... but maybe actually not, in this case. :) According to timsong-cpp.github.io, C++11 had
```
    explicit vector(const Allocator& = Allocator());
    explicit vector(size_type n);
    vector(size_type n, const T& value, const Allocator& = Allocator());
```
and C++14 had
```
    vector();
    explicit vector(const Allocator&);
    explicit vector(size_type n, const Allocator& = Allocator());
    vector(size_type n, const T& value, const Allocator& = Allocator());
```
That `(n, alloc)` ctor was added to C++14 via [LWG2210](https://cplusplus.github.io/LWG/issue2210).

Anyway, yeah, I think "value-initialize" is the correct answer. And I assume we agree that the extra move-construct is not observable.

If you're going to mess with the overload set here, now would be a //great// time to eliminate the default arguments entirely. ;)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D117108/new/

https://reviews.llvm.org/D117108