[libcxx-commits] [PATCH] D128146: [libc++] Use uninitialized algorithms for vector

[Hui via Phabricator via libcxx-commits]

huixie90 added a comment.

> @philnik , can you please help me understand what is going on with the original example posted by @bgraur ?
>
> The fact is that the example compiled before this patch and failed with this patch. Even if the example is buggy, I still need to understand what the semantics were when it worked. Was it using the wrong ctor? Why did it change with this patch?
>
> I'm seeing a number of tests failing with this patch, so understanding the example might give me a clue where to look.

In the example the constructor

  template <typename... Args>
  explicit Vx(Args&&... args)

is shadowing the default generated copy constructor `Vx(const Vx&) = default` when the `rhs` is not `const`
See  https://gcc.godbolt.org/z/3GYe9444P
If you have

  Vx<int, 5> v1{};
  Vx<int, 5> v2{v1};

The reason is that  `v1` is not `const`, on line 2, your constructor which takes `Args&&` is a better match then the `cosnt Vx&`, simply because the argument is not `const`.
And obviously your constructor cannot construct `Vx<int, 5>` with a `Vx<int, 5>`

However, if the caller uses a different syntax to trigger copy constructor

  Vx<int, 5> v1{};
  Vx<int, 5> v2 = v1;

https://gcc.godbolt.org/z/xffnrfWao
This works. Because unlike the previous way `Vx<int, 5> v2{v1};` explicitly call the copy constructor, this `Vx<int, 5> v2 = v1` implicitly calls the copy constructor.
You constructor is marked as `explicit` so in this case your constructor is not considered and the compiler will use the default generated copy constructor  `Vx(const Vx&) = default`.

So it might be that @philnik 's patch somehow changes the way to invoke copy constructor. In theory he could stick to original way, but relying on that sounds very fragile.
Usually if you need to write constructor that takes `Args&&...`, it is best practice to SFINAE out when `sizeof... == 1` and `remove_cvref_t<Arg>` is the class itself


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128146