This is the mail archive of the libstdc++@gcc.gnu.org mailing list for the libstdc++ project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: std::vector default default and move constructors


On 14/01/18 22:13 +0100, François Dumont wrote:
On 11/01/2018 07:57, Marc Glisse wrote:
On Thu, 11 Jan 2018, François Dumont wrote:

-       void _M_swap_data(_Vector_impl& __x) _GLIBCXX_NOEXCEPT
+       void
+       _M_swap_data(_Vector_impl_data& __x) _GLIBCXX_NOEXCEPT
        {
          std::swap(_M_start, __x._M_start);
          std::swap(_M_finish, __x._M_finish);
          std::swap(_M_end_of_storage, __x._M_end_of_storage);
        }


I don't remember earlier discussions about this patch, but is this piece of code still needed? std::swap(*this, __x) looks like it should work.

This patch has been arround for quite a long time now, initial submission was:

https://gcc.gnu.org/ml/libstdc++/2017-07/msg00060.html

even if I sent several updates since then.

I know, but it doesn't actually fix any bugs so has been low priority
and I've not been able to make time to finish reviewing it.

In fact it introduces a serious regression because of this line:

-      vector(vector&& __x) noexcept
-      : _Base(std::move(__x)) { }
+      vector(vector&&) = default;

Consider what happens if we have an allocator that is not
nothrow-move-constructible, which can happen if the allocator has a
noexcept(false) move constructor, or more likely just has no move
constructor but has a noexcept(false) copy constructor:

#include <vector>

template<typename T> struct Alloc : std::allocator<T> {
 Alloc() { }
 Alloc(const Alloc&) { }
 template<typename U> Alloc(const Alloc<U>&) { }
 template<typename U> struct rebind { using other = Alloc<U>; };
};

int main()
{
 static_assert( std::is_nothrow_move_constructible<std::vector<int, Alloc<int>>>::value, "" );
}

The standard says this program must compile, and with current GCC
trunk it does compile.

After your change it fails, because the vector(vector&&) constructor
is not explicitly noexcept, instead it deduces its exception
specification from its base classes, which includes the allocator.

So we have a patch which moves a load of code around without fixing
any bugs, but violates the requirements of the standard. I know you
want to simplify the code by making some things implicit, but I don't
think that's always better. If the standard says vector(vector&&) MUST
be noexcept, then making it implicit is not an improvement. If we say
things explicitly we can be sure they are correct, and can check them
more easily just by inspecting the code. When everything is implicit
you need to spend more time analysing the code to check it does the
right thing. I had to carefully review every constructor in the class
hierarchy to find this new bug, but the code you want to change
required no effort to check the exception-specification was correct:

-      vector(vector&& __x) noexcept



Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]