Re: [PATCH] PR77528 add default constructors for container adaptors

[Jonathan Wakely]

On 11/01/17 13:25 +, Jonathan Wakely wrote:

On 11/01/17 08:04 -0500, Tim Song wrote:

On Wed, Jan 11, 2017 at 7:21 AM, Jonathan Wakely  wrote:

This patch uses the _Enable_default_constructor mixin to properly
delete the default constructors. It's a bit cumbersome, because we
have to add an initializer for the base class to every
ctor-initializer-list, but I think I prefer this to making the default
constructor a constrained template.



I'm happy with either approach - my primary concern is making sure
that is_constructible and friends work and don't lie, in a world where
increasing numbers of library components depend on it. Though I'm a


Yes, it's important that we give the right answer.


bit curious as to why you found this approach more preferable.


I dislike making functions into templates when they aren't "supposed"
to be. But I'm in two minds for this case. It's certainly a smaller,
more self-contained change to just add a default constructor template
and not mess about with a new base class and DMIs and all those
mem-initializers.


Re the new DMI, my brain compiler says that _Sequence c = _Sequence();
breaks anything with an explicit copy/move constructor pre-C++17, but
I also don't think we care about those, right?


I dislike them, but maybe the fact they won't work here is a strong
enough reason to get over my dislike of the original approach and just
do it that way instead.


I decided to go with the original solution shown in the PR.

Tested powerpc64le-linux, committed to trunk.

commit b4614f1ef1a9c98650f2454332bb1407cddff13c
Author: Jonathan Wakely 
Date:   Thu Jan 12 15:37:27 2017 +

PR77528 partially revert r244278 and define default constructors

	PR libstdc++/77528
	* include/bits/stl_queue.h (queue, priority_queue): Remove default
	member-initializers and define default constructors as templates with
	constraints.
	* include/bits/stl_stack.h (stack): Likewise.
	* testsuite/23_containers/priority_queue/requirements/constructible.cc:
	New.
	* testsuite/23_containers/priority_queue/requirements/
	explicit_instantiation/1.cc: Test more instantiations.
	* testsuite/23_containers/priority_queue/requirements/
	explicit_instantiation/1_c++98.cc: Likewise.
	* testsuite/23_containers/queue/requirements/constructible.cc: New.
	* testsuite/23_containers/stack/requirements/constructible.cc: New.

diff --git a/libstdc++-v3/include/bits/stl_queue.h b/libstdc++-v3/include/bits/stl_queue.h
index 6417b30..3a52367 100644
--- a/libstdc++-v3/include/bits/stl_queue.h
+++ b/libstdc++-v3/include/bits/stl_queue.h
@@ -131,12 +131,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
*  of private: to allow derivation.  But none of the other
*  containers allow for derivation.  Odd.)
*/
-  /// @c c is the underlying container.
-#if __cplusplus >= 201103L
-  _Sequence c{};
-#else
+   ///  @c c is the underlying container.
   _Sequence c;
-#endif
 
 public:
   /**
@@ -147,7 +143,10 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
   queue(const _Sequence& __c = _Sequence())
   : c(__c) { }
 #else
-  queue() = default;
+  template::value>::type>
+	queue()
+	: c() { }
 
   explicit
   queue(const _Sequence& __c)
@@ -446,13 +445,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 
 protected:
   //  See queue::c for notes on these names.
-#if __cplusplus >= 201103L
-  _Sequence c{};
-  _Compare   comp{};
-#else
-  _Sequence c;
+  _Sequence  c;
   _Compare   comp;
-#endif
 
 public:
   /**
@@ -465,17 +459,19 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
   : c(__s), comp(__x)
   { std::make_heap(c.begin(), c.end(), comp); }
 #else
-  priority_queue() = default;
+  template,
+is_default_constructible<_Seq>>::value>::type>
+	priority_queue()
+	: c(), comp() { }
 
   explicit
-  priority_queue(const _Compare& __x,
-		 const _Sequence& __s)
+  priority_queue(const _Compare& __x, const _Sequence& __s)
   : c(__s), comp(__x)
   { std::make_heap(c.begin(), c.end(), comp); }
 
   explicit
-  priority_queue(const _Compare& __x,
-		 _Sequence&& __s = _Sequence())
+  priority_queue(const _Compare& __x, _Sequence&& __s = _Sequence())
   : c(std::move(__s)), comp(__x)
   { std::make_heap(c.begin(), c.end(), comp); }
 
diff --git a/libstdc++-v3/include/bits/stl_stack.h b/libstdc++-v3/include/bits/stl_stack.h
index a0f9ee5..094ce65 100644
--- a/libstdc++-v3/include/bits/stl_stack.h
+++ b/libstdc++-v3/include/bits/stl_stack.h
@@ -129,11 +129,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 
 protected:
   //  See queue::c for notes on this name.
-#if __cplusplus >= 201103L
-  _Sequence c{};
-#else
   _Sequence c;
-#endif
 
 public:
   // XXX removed old def ctor, added def arg to this one to match 14882
@@ -145,7 +141,10 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 

Re: [PATCH] PR77528 add default constructors for container adaptors

[Tim Song]

On Wed, Jan 11, 2017 at 8:30 AM, Jonathan Wakely  wrote:

>>> Re the new DMI, my brain compiler says that _Sequence c = _Sequence();
>>> breaks anything with an explicit copy/move constructor pre-C++17, but
>>> I also don't think we care about those, right?
>>
>>
>> I dislike them,
>
>
> I meant to add "but we try to support them where plausible".
>
> If the standard requires CopyConstructible then we don't need to care
> about explicit copy constructors. But if it only requires
> is_copy_constructible then that does work with explicit copy ctors.
> And if it says one thing, but means the other, then we just have to
> guess what's intended! :-)
>
>

Clause 23 is ... not a model of clarity. It depends on how strongly
you read the "any sequence container" phrasing, I suppose. Table 83
requires X u = a; to work for containers, but it also requires a == b
to work.

There's also the problem of Compare (which I don't see any requirement
w/r/t CopyConstructible and like on). It does say things like
"initializes comp with x", but doesn't say what kind of
initialization...

Re: [PATCH] PR77528 add default constructors for container adaptors

[Jonathan Wakely]

On 11/01/17 13:25 +, Jonathan Wakely wrote:

On 11/01/17 08:04 -0500, Tim Song wrote:

On Wed, Jan 11, 2017 at 7:21 AM, Jonathan Wakely  wrote:

This patch uses the _Enable_default_constructor mixin to properly
delete the default constructors. It's a bit cumbersome, because we
have to add an initializer for the base class to every
ctor-initializer-list, but I think I prefer this to making the default
constructor a constrained template.



I'm happy with either approach - my primary concern is making sure
that is_constructible and friends work and don't lie, in a world where
increasing numbers of library components depend on it. Though I'm a


Yes, it's important that we give the right answer.


bit curious as to why you found this approach more preferable.


I dislike making functions into templates when they aren't "supposed"
to be. But I'm in two minds for this case. It's certainly a smaller,
more self-contained change to just add a default constructor template
and not mess about with a new base class and DMIs and all those
mem-initializers.


Re the new DMI, my brain compiler says that _Sequence c = _Sequence();
breaks anything with an explicit copy/move constructor pre-C++17, but
I also don't think we care about those, right?


I dislike them,


I meant to add "but we try to support them where plausible".

If the standard requires CopyConstructible then we don't need to care
about explicit copy constructors. But if it only requires
is_copy_constructible then that does work with explicit copy ctors.
And if it says one thing, but means the other, then we just have to
guess what's intended! :-)


but maybe the fact they won't work here is a strong
enough reason to get over my dislike of the original approach and just
do it that way instead.

Re: [PATCH] PR77528 add default constructors for container adaptors

[Jonathan Wakely]

On 11/01/17 08:04 -0500, Tim Song wrote:

On Wed, Jan 11, 2017 at 7:21 AM, Jonathan Wakely  wrote:

This patch uses the _Enable_default_constructor mixin to properly
delete the default constructors. It's a bit cumbersome, because we
have to add an initializer for the base class to every
ctor-initializer-list, but I think I prefer this to making the default
constructor a constrained template.



I'm happy with either approach - my primary concern is making sure
that is_constructible and friends work and don't lie, in a world where
increasing numbers of library components depend on it. Though I'm a


Yes, it's important that we give the right answer.


bit curious as to why you found this approach more preferable.


I dislike making functions into templates when they aren't "supposed"
to be. But I'm in two minds for this case. It's certainly a smaller,
more self-contained change to just add a default constructor template
and not mess about with a new base class and DMIs and all those
mem-initializers.


Re the new DMI, my brain compiler says that _Sequence c = _Sequence();
breaks anything with an explicit copy/move constructor pre-C++17, but
I also don't think we care about those, right?


I dislike them, but maybe the fact they won't work here is a strong
enough reason to get over my dislike of the original approach and just
do it that way instead.

Re: [PATCH] PR77528 add default constructors for container adaptors

[Tim Song]

On Wed, Jan 11, 2017 at 7:21 AM, Jonathan Wakely  wrote:
> This patch uses the _Enable_default_constructor mixin to properly
> delete the default constructors. It's a bit cumbersome, because we
> have to add an initializer for the base class to every
> ctor-initializer-list, but I think I prefer this to making the default
> constructor a constrained template.
>

I'm happy with either approach - my primary concern is making sure
that is_constructible and friends work and don't lie, in a world where
increasing numbers of library components depend on it. Though I'm a
bit curious as to why you found this approach more preferable.

Re the new DMI, my brain compiler says that _Sequence c = _Sequence();
breaks anything with an explicit copy/move constructor pre-C++17, but
I also don't think we care about those, right?

Re: [PATCH] PR77528 add default constructors for container adaptors

[Jonathan Wakely]

On 10/01/17 13:15 -0500, Tim Song wrote:

On Tue, Jan 10, 2017 at 12:33 PM, Jonathan Wakely  wrote:

The standard says that the container adaptors have a constructor with
a default argument, which serves as a default constructor. That
involves default-constructing the underlying sequence as the default
argument and then move-constructing the member variable from that
argument. Because std::deque allocates memory in its move constructor
this means the default constructor of an adaptor using std::deque
will allocate twice, which is wasteful and expensive.

This change adds a separate default constructor, defined as defaulted
(and adding default member-initializers to ensure the member variables
get value-initialized). This avoids the move-construction, so we only
allocate once when using std::deque.



The new default member initializers use {}, and it's not too hard to find
test cases where {} and value-initialization do different things, including
cases where {} doesn't compile but () does. (So much for "uniform"
initialization.)


OK that's easily fixed.


Because the default constructor is defined as defaulted it will be
deleted when the underlying sequence isn't default constructible,


That's not correct. There's no implicit deletion due to the presence of DMIs.
The reason the explicit instantiation works is that constructors explicitly
defaulted at their first declaration are not implicitly defined until ODR-used.

So, unlike the SFINAE-based approach outlined in the bugzilla issue, this
patch causes is_default_constructible>
to trigger a hard error (though the standard's version isn't SFINAE-friendly
either).


Oops, thanks.


Also, the change to .../priority_queue/requirements/explicit_instantiation/1.cc
adds a Cmp class but doesn't actually use it in the explicit instantiation.


Fixed.

This patch uses the _Enable_default_constructor mixin to properly
delete the default constructors. It's a bit cumbersome, because we
have to add an initializer for the base class to every
ctor-initializer-list, but I think I prefer this to making the default
constructor a constrained template.

This also includes tests for is_default_constructible to ensure we
don't get the same hard errors as the previous version.

I'm still testing this, and could be persuaded to go with the
constrained templates if there's a good reason to.

commit 2bd10a76508336d92decfefe858fc23c5bc272f0
Author: Jonathan Wakely 
Date:   Wed Jan 11 12:15:27 2017 +

PR77528 conditionally delete default constructors

	PR libstdc++/77528
	* include/bits/stl_queue.h (queue, priority_queue): Use derivation
	from _Enable_default_constructor mixin to conditionally delete disable
	default construction
	* include/bits/stl_stack.h (stack): Likewise.
	* testsuite/23_containers/priority_queue/requirements/constructible.cc:
	New.
	* testsuite/23_containers/priority_queue/requirements/
	explicit_instantiation/1.cc: Test more instantiations.
	* testsuite/23_containers/priority_queue/requirements/
	explicit_instantiation/1_c++98.cc: Likewise.
	* testsuite/23_containers/queue/requirements/constructible.cc: New.
	* testsuite/23_containers/stack/requirements/constructible.cc: New.

diff --git a/libstdc++-v3/include/bits/stl_queue.h b/libstdc++-v3/include/bits/stl_queue.h
index 6417b30..ebd6089 100644
--- a/libstdc++-v3/include/bits/stl_queue.h
+++ b/libstdc++-v3/include/bits/stl_queue.h
@@ -60,6 +60,7 @@
 #include 
 #if __cplusplus >= 201103L
 # include 
+# include 
 #endif
 
 namespace std _GLIBCXX_VISIBILITY(default)
@@ -94,6 +95,10 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
   */
   template >
 class queue
+#if __cplusplus >= 201103L
+: _Enable_default_constructor::value,
+  queue<_Tp, _Sequence>>
+#endif
 {
   // concept requirements
   typedef typename _Sequence::value_type _Sequence_value_type;
@@ -114,6 +119,10 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
   template
 	using _Uses = typename
 	  enable_if::value>::type;
+
+  using _Tag = _Enable_default_constructor_tag;
+  using _Base = _Enable_default_constructor<
+	is_default_constructible<_Sequence>::value, queue>;
 #endif
 
 public:
@@ -133,7 +142,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
*/
   /// @c c is the underlying container.
 #if __cplusplus >= 201103L
-  _Sequence c{};
+  _Sequence c = _Sequence();
 #else
   _Sequence c;
 #endif
@@ -151,32 +160,32 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 
   explicit
   queue(const _Sequence& __c)
-  : c(__c) { }
+  : _Base(_Tag()), c(__c) { }
 
   explicit
   queue(_Sequence&& __c)
-  : c(std::move(__c)) { }
+  : _Base(_Tag()), c(std::move(__c)) { }
 
   template>
 	explicit
 	queue(const _Alloc& __a)
-	: c(__a) { }
+	: _Base(_Tag()), c(__a) { }
 
   template>
 	queue(const 

Re: [PATCH] PR77528 add default constructors for container adaptors

[Tim Song]

On Tue, Jan 10, 2017 at 12:33 PM, Jonathan Wakely  wrote:
> The standard says that the container adaptors have a constructor with
> a default argument, which serves as a default constructor. That
> involves default-constructing the underlying sequence as the default
> argument and then move-constructing the member variable from that
> argument. Because std::deque allocates memory in its move constructor
> this means the default constructor of an adaptor using std::deque
> will allocate twice, which is wasteful and expensive.
>
> This change adds a separate default constructor, defined as defaulted
> (and adding default member-initializers to ensure the member variables
> get value-initialized). This avoids the move-construction, so we only
> allocate once when using std::deque.
>

The new default member initializers use {}, and it's not too hard to find
test cases where {} and value-initialization do different things, including
cases where {} doesn't compile but () does. (So much for "uniform"
initialization.)

> Because the default constructor is defined as defaulted it will be
> deleted when the underlying sequence isn't default constructible,

That's not correct. There's no implicit deletion due to the presence of DMIs.
The reason the explicit instantiation works is that constructors explicitly
defaulted at their first declaration are not implicitly defined until ODR-used.

So, unlike the SFINAE-based approach outlined in the bugzilla issue, this
patch causes is_default_constructible>
to trigger a hard error (though the standard's version isn't SFINAE-friendly
either).

Also, the change to .../priority_queue/requirements/explicit_instantiation/1.cc
adds a Cmp class but doesn't actually use it in the explicit instantiation.

[PATCH] PR77528 add default constructors for container adaptors

[Jonathan Wakely]

The standard says that the container adaptors have a constructor with
a default argument, which serves as a default constructor. That
involves default-constructing the underlying sequence as the default
argument and then move-constructing the member variable from that
argument. Because std::deque allocates memory in its move constructor
this means the default constructor of an adaptor using std::deque
will allocate twice, which is wasteful and expensive.

This change adds a separate default constructor, defined as defaulted
(and adding default member-initializers to ensure the member variables
get value-initialized). This avoids the move-construction, so we only
allocate once when using std::deque.

As noted in Bugzilla this makes the default constructors non-explicit,
which is a change in behaviour, and not strictly conforming. I'm doing
that intentionally, because having default construction be explicit
for these types is stupid anyway.

Because the default constructor is defined as defaulted it will be
deleted when the underlying sequence isn't default constructible, so
we can still explicitly instantiate the adaptors in such cases (which
I've added tests for).

I'm also shuffling some tests around. Currently we have two explicit
instantiation tests for each adaptor, 1.cc and 1_c++0x.cc, which
originally tested it for C++98 and C++11. But since the default became
gnu++14 we now test twice in C++14 mode. So I'm replacing 1_c++0x.cc
with 1_c++98.cc which has { dg-options "-std=gnu++98.cc" } to force
testing that dialect.

PR libstdc++/77528
* include/bits/stl_queue.h (queue::c): Add default member initializer.
(queue::queue()): Add constructor and define as defaulted.
(queue::queue(_Sequence&&)): Remove default argument.
(priority_queue::c, priority_queue::comp): Add default member
initializers.
(priority_queue::priority_queue()): Add constructor and define as
defaulted.
(priority_queue::priority_queue(const _Compare&, _Sequence&&)):
Remove default argument for first parameter.
* include/bits/stl_stack.h (stack::c): Add default member initializer.
(stack::stack()): Add constructor and define as defaulted.
(stack::stack(const _Sequence&)): Remove default argument.
* testsuite/23_containers/priority_queue/requirements/
explicit_instantiation/1.cc: Test explicit instantiation with
non-DefaultConstructible sequence.
* testsuite/23_containers/priority_queue/77528.cc: New test.
* testsuite/23_containers/priority_queue/requirements/
explicit_instantiation/1_c++0x.cc: Replace with 1_c++98.cc.
* testsuite/23_containers/queue/77528.cc: New test.
* testsuite/23_containers/queue/requirements/explicit_instantiation/
1.cc: Test explicit instantiation with non-DefaultConstructible
sequence.
* testsuite/23_containers/queue/requirements/explicit_instantiation/
1_c++0x.cc: Replace with 1_c++98.cc.
* testsuite/23_containers/stack/77528.cc: New test.
* testsuite/23_containers/stack/requirements/explicit_instantiation/
1.cc: Test explicit instantiation with non-DefaultConstructible
sequence.
* testsuite/23_containers/stack/requirements/explicit_instantiation/
1_c++0x.cc: Replace with 1_c++98.cc.

Tested powerpc64le-linux, committed to trunk.

We might want to backport this to the branches, but I'll let it bake
on trunk for a bit first.

commit 5fe45739d39c67fae0f9a3179c2f61b6c95428e4
Author: Jonathan Wakely 
Date:   Thu Sep 8 14:32:37 2016 +0100

PR77528 add default constructors for container adaptors

PR libstdc++/77528
* include/bits/stl_queue.h (queue::c): Add default member initializer.
(queue::queue()): Add constructor and define as defaulted.
(queue::queue(_Sequence&&)): Remove default argument.
(priority_queue::c, priority_queue::comp): Add default member
initializers.
(priority_queue::priority_queue()): Add constructor and define as
defaulted.
(priority_queue::priority_queue(const _Compare&, _Sequence&&)):
Remove default argument for first parameter.
* include/bits/stl_stack.h (stack::c): Add default member initializer.
(stack::stack()): Add constructor and define as defaulted.
(stack::stack(const _Sequence&)): Remove default argument.
* testsuite/23_containers/priority_queue/requirements/
explicit_instantiation/1.cc: Test explicit instantiation with
non-DefaultConstructible sequence.
* testsuite/23_containers/priority_queue/77528.cc: New test.
* testsuite/23_containers/priority_queue/requirements/
explicit_instantiation/1_c++0x.cc: Replace with 1_c++98.cc.
* testsuite/23_containers/queue/77528.cc: New test.
* testsuite/23_containers/queue/requirements/explicit_instantiation/