This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: Containers default initialization
On 08/06/2017 15:22, Jonathan Wakely wrote:
Oh dear, we have a compiler bug:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65816
And not a recent one !
which takes place only if I call this:
test_type *tmp = ::new(buf._M_addr()) test_type {};
To force default/value init looks like gcc forces you to explicitly
build an allocator instance like in the attached patch.
I'm getting uncomfortable with all this churn just to be able to use
=default, but requiring more new base classes and #if blocks. We have
one known regression already, which needs your patch to fix it.
(Admittedly, it's not a very serious regression, because I think real
world allocators that are stateful but don't have user-provided
default constructors are probably very very unlikely).
I'm leaning towards just make the exception specifications correct
using _GLIBCXX_NOEXCEPT_IF and not doing any refactoring.
I prefer this approach because it split responsibilities. I like the one
class, one responsibility approach. It also avoids repeating the data
members initialization in several constructors when just a default one
is needed. And it helps making the move constructor use the default
implementation.
The purpose of this refactoring was also to bring together the noexcept
qualification with the operation potentially raising it. It was strange
to have the noexcept on one constructor but the allocator default
constructor call at another level.
That also has the benefit that the code might help guide the future
direction of the standard, by telling us what conditions could be used
if we add noexcept to these constructors in the standard.
If it was for doc purpose Doxygen could perhaps help, with gcc help I
guess. But to fix the regression we need to restore the explicit
noexcept quaification so we are all happy :-).
Index: include/bits/stl_tree.h
===================================================================
--- include/bits/stl_tree.h (revision 248855)
+++ include/bits/stl_tree.h (working copy)
@@ -687,9 +687,17 @@
#if __cplusplus < 201103L
_Rb_tree_impl()
+ : _Node_allocator()
{ }
#else
- _Rb_tree_impl() = default;
+ _Rb_tree_impl()
+ noexcept(
+ noexcept(_Node_allocator()) && noexcept(_Base_key_compare()) )
+ : _Rb_tree_impl(_Key_compare(), _Node_allocator())
If we didn't have the PR65816 bug this should be simply:
_Rb_tree_impl()
_GLIBCXX_NOEXCEPT_IF(
is_nothrow_default_constructible<_Key_compare>::value
&& is_nothrow_default_constructible<_Node_allocator>::value
)
: _Node_allocator()
{ }
i.e. the same for C++98 and other modes. Because of the compiler bug
that doesn't work. I think we should fix the compiler.
Yes, that's reassuring, I much prefer this too.
+ { }
+#endif
+
+#if __cplusplus >= 201103L
There's no need for an #endif/#if here, the previous block is already
C++11-and-later code.
+ __gnu_cxx::__aligned_buffer<test_type> buf;
+ __builtin_memset(buf._M_addr(), ~0, sizeof(test_type));
+
+ VERIFY( buf._M_ptr()->get_allocator().state != 0 );
This is undefined. There is no object here, so you can't dereference
the value returned by bug._M_ptr().
I wanted to make sure that the compiler was not going to optimize the
memset away as he could. But in -O0 I guess compiler won't do it.
+ test_type *tmp = ::new(buf._M_addr()) test_type();
This file is called default_init.cc but this is value-initialization.
Oh, I was seeing things the wrong way so.
Can't we just have one file per container type (maybe just called
default_init.cc) which tests default-initialization in test01() and
value-initialization in a test02() function?
While working on this weird behavior I wanted to always test the 2 types
of init. If they were in the same file the 1st failure would stop the
tests. Do you really want them in the same file ?
But is there any dg-xxx flag to put to indicate a failing test for known
reason ? Or should I simply keep the test on my desktop waiting for the
compiler fix ?
Attach is the new patch tested under Linux x86_64 normal mode. Note that
I added a simplification of the _M_get_Node_allocator(), static_cast is
useless no ?
Ok to commit ?
François
diff --git a/libstdc++-v3/include/bits/stl_tree.h b/libstdc++-v3/include/bits/stl_tree.h
index 2161e05..984801b 100644
--- a/libstdc++-v3/include/bits/stl_tree.h
+++ b/libstdc++-v3/include/bits/stl_tree.h
@@ -610,11 +610,11 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
_Node_allocator&
_M_get_Node_allocator() _GLIBCXX_NOEXCEPT
- { return *static_cast<_Node_allocator*>(&this->_M_impl); }
+ { return this->_M_impl; }
const _Node_allocator&
_M_get_Node_allocator() const _GLIBCXX_NOEXCEPT
- { return *static_cast<const _Node_allocator*>(&this->_M_impl); }
+ { return this->_M_impl; }
allocator_type
get_allocator() const _GLIBCXX_NOEXCEPT
@@ -723,13 +723,12 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
{
typedef _Rb_tree_key_compare<_Key_compare> _Base_key_compare;
-#if __cplusplus < 201103L
_Rb_tree_impl()
+ _GLIBCXX_NOEXCEPT_IF(
+ is_nothrow_default_constructible<_Node_allocator>::value
+ && is_nothrow_default_constructible<_Base_key_compare>::value )
+ : _Node_allocator()
{ }
-#else
- _Rb_tree_impl() = default;
- _Rb_tree_impl(_Rb_tree_impl&&) = default;
-#endif
_Rb_tree_impl(const _Rb_tree_impl& __x)
: _Node_allocator(_Alloc_traits::_S_select_on_copy(__x))
@@ -741,6 +740,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
: _Node_allocator(__a), _Base_key_compare(__comp)
{ }
#else
+ _Rb_tree_impl(_Rb_tree_impl&&) = default;
+
_Rb_tree_impl(const _Key_compare& __comp, _Node_allocator&& __a)
: _Node_allocator(std::move(__a)), _Base_key_compare(__comp)
{ }
diff --git a/libstdc++-v3/testsuite/23_containers/map/allocator/default_init.cc b/libstdc++-v3/testsuite/23_containers/map/allocator/default_init.cc
new file mode 100644
index 0000000..f991dec
--- /dev/null
+++ b/libstdc++-v3/testsuite/23_containers/map/allocator/default_init.cc
@@ -0,0 +1,50 @@
+// Copyright (C) 2017 Free Software Foundation, Inc.
+//
+// This file is part of the GNU ISO C++ Library. This library is free
+// software; you can redistribute it and/or modify it under the
+// terms of the GNU General Public License as published by the
+// Free Software Foundation; either version 3, or (at your option)
+// any later version.
+
+// This library is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+// GNU General Public License for more details.
+
+// You should have received a copy of the GNU General Public License along
+// with this library; see the file COPYING3. If not see
+// <http://www.gnu.org/licenses/>.
+
+// { dg-do run { target c++11 } }
+// { dg-options "-O0" }
+
+#include <map>
+#include <testsuite_hooks.h>
+#include <testsuite_allocator.h>
+
+#include <ext/aligned_buffer.h>
+
+using T = int;
+
+using __gnu_test::default_init_allocator;
+
+void test01()
+{
+ typedef default_init_allocator<std::pair<const T, T>> alloc_type;
+ typedef std::map<T, T, std::less<T>, alloc_type> test_type;
+
+ __gnu_cxx::__aligned_buffer<test_type> buf;
+ __builtin_memset(buf._M_addr(), ~0, sizeof(test_type));
+
+ test_type *tmp = ::new(buf._M_addr()) test_type;
+
+ VERIFY( tmp->get_allocator().state == 0 );
+
+ tmp->~test_type();
+}
+
+int main()
+{
+ test01();
+ return 0;
+}
diff --git a/libstdc++-v3/testsuite/23_containers/map/allocator/value_init.cc b/libstdc++-v3/testsuite/23_containers/map/allocator/value_init.cc
new file mode 100644
index 0000000..48b6cbb
--- /dev/null
+++ b/libstdc++-v3/testsuite/23_containers/map/allocator/value_init.cc
@@ -0,0 +1,50 @@
+// Copyright (C) 2017 Free Software Foundation, Inc.
+//
+// This file is part of the GNU ISO C++ Library. This library is free
+// software; you can redistribute it and/or modify it under the
+// terms of the GNU General Public License as published by the
+// Free Software Foundation; either version 3, or (at your option)
+// any later version.
+
+// This library is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+// GNU General Public License for more details.
+
+// You should have received a copy of the GNU General Public License along
+// with this library; see the file COPYING3. If not see
+// <http://www.gnu.org/licenses/>.
+
+// { dg-do run { target c++11 } }
+// { dg-options "-O0" }
+
+#include <map>
+#include <testsuite_hooks.h>
+#include <testsuite_allocator.h>
+
+#include <ext/aligned_buffer.h>
+
+using T = int;
+
+using __gnu_test::default_init_allocator;
+
+void test01()
+{
+ typedef default_init_allocator<std::pair<const T, T>> alloc_type;
+ typedef std::map<T, T, std::less<T>, alloc_type> test_type;
+
+ __gnu_cxx::__aligned_buffer<test_type> buf;
+ __builtin_memset(buf._M_addr(), ~0, sizeof(test_type));
+
+ test_type *tmp = ::new(buf._M_addr()) test_type();
+
+ VERIFY( tmp->get_allocator().state == 0 );
+
+ tmp->~test_type();
+}
+
+int main()
+{
+ test01();
+ return 0;
+}
diff --git a/libstdc++-v3/testsuite/23_containers/set/allocator/default_init.cc b/libstdc++-v3/testsuite/23_containers/set/allocator/default_init.cc
new file mode 100644
index 0000000..789c687
--- /dev/null
+++ b/libstdc++-v3/testsuite/23_containers/set/allocator/default_init.cc
@@ -0,0 +1,50 @@
+// Copyright (C) 2017 Free Software Foundation, Inc.
+//
+// This file is part of the GNU ISO C++ Library. This library is free
+// software; you can redistribute it and/or modify it under the
+// terms of the GNU General Public License as published by the
+// Free Software Foundation; either version 3, or (at your option)
+// any later version.
+
+// This library is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+// GNU General Public License for more details.
+
+// You should have received a copy of the GNU General Public License along
+// with this library; see the file COPYING3. If not see
+// <http://www.gnu.org/licenses/>.
+
+// { dg-do run { target c++11 } }
+// { dg-options "-O0" }
+
+#include <set>
+#include <testsuite_hooks.h>
+#include <testsuite_allocator.h>
+
+#include <ext/aligned_buffer.h>
+
+using T = int;
+
+using __gnu_test::default_init_allocator;
+
+void test01()
+{
+ typedef default_init_allocator<T> alloc_type;
+ typedef std::set<T, std::less<T>, alloc_type> test_type;
+
+ __gnu_cxx::__aligned_buffer<test_type> buf;
+ __builtin_memset(buf._M_addr(), ~0, sizeof(test_type));
+
+ test_type *tmp = ::new(buf._M_addr()) test_type;
+
+ VERIFY( tmp->get_allocator().state == 0 );
+
+ tmp->~test_type();
+}
+
+int main()
+{
+ test01();
+ return 0;
+}
diff --git a/libstdc++-v3/testsuite/23_containers/set/allocator/value_init.cc b/libstdc++-v3/testsuite/23_containers/set/allocator/value_init.cc
new file mode 100644
index 0000000..9a768c7
--- /dev/null
+++ b/libstdc++-v3/testsuite/23_containers/set/allocator/value_init.cc
@@ -0,0 +1,50 @@
+// Copyright (C) 2017 Free Software Foundation, Inc.
+//
+// This file is part of the GNU ISO C++ Library. This library is free
+// software; you can redistribute it and/or modify it under the
+// terms of the GNU General Public License as published by the
+// Free Software Foundation; either version 3, or (at your option)
+// any later version.
+
+// This library is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+// GNU General Public License for more details.
+
+// You should have received a copy of the GNU General Public License along
+// with this library; see the file COPYING3. If not see
+// <http://www.gnu.org/licenses/>.
+
+// { dg-do run { target c++11 } }
+// { dg-options "-O0" }
+
+#include <set>
+#include <testsuite_hooks.h>
+#include <testsuite_allocator.h>
+
+#include <ext/aligned_buffer.h>
+
+using T = int;
+
+using __gnu_test::default_init_allocator;
+
+void test01()
+{
+ typedef default_init_allocator<T> alloc_type;
+ typedef std::set<T, std::less<T>, alloc_type> test_type;
+
+ __gnu_cxx::__aligned_buffer<test_type> buf;
+ __builtin_memset(buf._M_addr(), ~0, sizeof(test_type));
+
+ test_type *tmp = ::new(buf._M_addr()) test_type();
+
+ VERIFY( tmp->get_allocator().state == 0 );
+
+ tmp->~test_type();
+}
+
+int main()
+{
+ test01();
+ return 0;
+}