[Bug libstdc++/51183] New: pair piecewise_construct_t constructor copies

[Bug libstdc++/51183] New: pair piecewise_construct_t constructor copies

[marc.glisse at normalesup dot org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=51183

             Bug #: 51183
           Summary: pair piecewise_construct_t constructor copies
    Classification: Unclassified
           Product: gcc
           Version: 4.7.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: libstdc++
        AssignedTo: unassigned@gcc.gnu.org
        ReportedBy: marc.glisse@normalesup.org


Hello,

on clc++, someone noticed that the technique used by libstdc++ for pair's
piecewise_construct_t constructor calls a (elided) copy constructor. In
particular:

#include <utility>
#include <tuple>
struct A {
        A();
        A(A const&)=delete;
};
int main(){
        std::pair<A,int> p { std::piecewise_construct_t(), std::make_tuple(),
std::make_tuple(1) };
}

is rejected by the compiler.

[Bug libstdc++/51183] pair piecewise_construct_t constructor copies

[chris at bubblescope dot net]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=51183

Chris Jefferson <chris at bubblescope dot net> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |chris at bubblescope dot
                   |                            |net

--- Comment #1 from Chris Jefferson <chris at bubblescope dot net> 2011-11-17 09:11:02 UTC ---
This was necessary because (I believe) it is impossible to implement the
piecewise_construct_t constructor without compiler support for forwarding
constructors. Last time I checked, they hadn't been implemented yet.

If they have been implemented, I can supply the code to make this work
correctly.

[Bug libstdc++/51183] pair piecewise_construct_t constructor copies

[redi at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=51183

--- Comment #2 from Jonathan Wakely <redi at gcc dot gnu.org> 2011-11-17 09:30:41 UTC ---
nope, they haven't

[Bug libstdc++/51183] pair piecewise_construct_t constructor copies

[redi at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=51183

Jonathan Wakely <redi at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
   Last reconfirmed|                            |2011-11-17
         Depends on|                            |43674
     Ever Confirmed|0                           |1

--- Comment #3 from Jonathan Wakely <redi at gcc dot gnu.org> 2011-11-17 10:19:14 UTC ---
I've updated the C++11 status table in the manual to say that piecewise
construction requires an accessible copy/move constructor, and marked this as
dependent on PR 43674

[Bug libstdc++/51183] pair piecewise_construct_t constructor copies

[marc.glisse at normalesup dot org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=51183

--- Comment #4 from Marc Glisse <marc.glisse at normalesup dot org> 2011-11-17 11:44:55 UTC ---
(In reply to comment #1)
> This was necessary because (I believe) it is impossible to implement the
> piecewise_construct_t constructor without compiler support for forwarding
> constructors.

Ah, right, makes sense, thank you.

[Bug libstdc++/51183] pair piecewise_construct_t constructor copies

[paolo.carlini at oracle dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=51183

Paolo Carlini <paolo.carlini at oracle dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |paolo.carlini at oracle dot
                   |                            |com

--- Comment #5 from Paolo Carlini <paolo.carlini at oracle dot com> 2011-12-05 15:54:05 UTC ---
Chris, delegating constructors are just in, a patch would be welcome...

[Bug libstdc++/51183] pair piecewise_construct_t constructor copies

[redi at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=51183

--- Comment #6 from Jonathan Wakely <redi at gcc dot gnu.org> 2011-12-05 16:06:57 UTC ---
Nice!

it should be pretty simple:

      template<class... _Args1, class... _Args2>
        pair(piecewise_construct_t,
             tuple<_Args1...> __first, tuple<_Args2...> __second)
        : pair(std::move(__first), std::move(__second),
               typename _Build_index_tuple<sizeof...(_Args1)>::__type(),
               typename _Build_index_tuple<sizeof...(_Args2)>::__type())
        { }


      template<typename... _Args2, std::size_t... _Indexes1,
               typename... _Args2, std::size_t... _Indexes2>
        explicit
        pair(tuple<_Args1...> __tuple1, tuple<_Args2> __tuple2,
             _Index_tuple<_Indexes1...>, _Index_tuple<_Indexes2...>)
        : first(std::forward<_Args1>(get<_Indexes1>(__tuple1))...),
          second(std::forward<_Args2>(get<_Indexes2>(__tuple2))...)
        { }

[Bug libstdc++/51183] pair piecewise_construct_t constructor copies

[redi at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=51183

--- Comment #7 from Jonathan Wakely <redi at gcc dot gnu.org> 2011-12-05 16:09:18 UTC ---
(In reply to comment #6)
>       template<typename... _Args2, std::size_t... _Indexes1,
                             ^^^^^^
                   should be _Args1

that's what I had prototyped a couple of weeks ago, it's untested obviously

[Bug libstdc++/51183] pair piecewise_construct_t constructor copies

[paolo.carlini at oracle dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=51183

--- Comment #8 from Paolo Carlini <paolo.carlini at oracle dot com> 2011-12-05 16:12:35 UTC ---
I suspected that. Thus, Jon, if you like, just test and commit! ;)

[Bug libstdc++/51183] pair piecewise_construct_t constructor copies

[chris at bubblescope dot net]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=51183

--- Comment #9 from Chris Jefferson <chris at bubblescope dot net> 2011-12-05 16:19:40 UTC ---
The only difference in the version I wrote was that I passed the arguments into
the explicit constructor as non-const references, rather than by value with
std::move, which should be more efficent for types without a move constructor.
However, there really isn't very much choice in how you implement this.

Jonathan, if you are already off writing tests and ready to commit, I'm happy
to let you finish, else I could write some tests and submit a patch.

[Bug libstdc++/51183] pair piecewise_construct_t constructor copies

[chris at bubblescope dot net]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=51183

--- Comment #10 from Chris Jefferson <chris at bubblescope dot net> 2011-12-05 16:25:13 UTC ---
Oh, and one other tiny detail, I've about given up trying to understand corner
cases in the name look-up rules in C++, so I'd probably std:: qualify those
'get's, just to be on the safe side. Although I think this is one case where
things are OK.

[Bug libstdc++/51183] pair piecewise_construct_t constructor copies

[redi at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=51183

--- Comment #11 from Jonathan Wakely <redi at gcc dot gnu.org> 2011-12-05 16:27:07 UTC ---
I was assuming that since the public piecewise constructor takes them by value
the extra move would be elided ... that might not be true though.

I haven't written any tests for it and have a few other patches I need to chase
for approval, so if you have time to work on this please be my guest, thanks!

[Bug libstdc++/51183] pair piecewise_construct_t constructor copies

[paolo.carlini at oracle dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=51183

Paolo Carlini <paolo.carlini at oracle dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|paolo.carlini at oracle dot |
                   |com                         |
         AssignedTo|unassigned at gcc dot       |paolo.carlini at oracle dot
                   |gnu.org                     |com
   Target Milestone|---                         |4.7.0

--- Comment #12 from Paolo Carlini <paolo.carlini at oracle dot com> 2011-12-06 09:55:45 UTC ---
I'm taking care of this.

[Bug libstdc++/51183] pair piecewise_construct_t constructor copies

[paolo.carlini at oracle dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=51183

Paolo Carlini <paolo.carlini at oracle dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |ASSIGNED

[Bug libstdc++/51183] pair piecewise_construct_t constructor copies

[chris at bubblescope dot net]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=51183

--- Comment #13 from Chris Jefferson <chris at bubblescope dot net> 2011-12-06 10:13:56 UTC ---
You can if you like, but if you haven't started yet, I plan on having a patch
ready in about... 2 hours?

[Bug libstdc++/51183] pair piecewise_construct_t constructor copies

[paolo.carlini at oracle dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=51183

--- Comment #15 from Paolo Carlini <paolo.carlini at oracle dot com> 2011-12-06 10:31:30 UTC ---
Created attachment 26005
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=26005
Tested patch

[Bug libstdc++/51183] pair piecewise_construct_t constructor copies

[paolo.carlini at oracle dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=51183

--- Comment #14 from Paolo Carlini <paolo.carlini at oracle dot com> 2011-12-06 10:30:42 UTC ---
I'm attaching what I already tested and was going to commit. If you like,
please work on top of it and produce a combined new patch. Like, if you think
another testcase is necessary or, if the assembly supports you, you believe
non-const ref passing is actually useful.

I'd like to resolve this issue today and move on. Thanks!

[Bug libstdc++/51183] pair piecewise_construct_t constructor copies

[chris at bubblescope dot net]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=51183

--- Comment #16 from Chris Jefferson <chris at bubblescope dot net> 2011-12-06 14:25:24 UTC ---
Created attachment 26006
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=26006
Piecewise patch

Patch to make piecewise_construct work properly on std::pair.

[Bug libstdc++/51183] pair piecewise_construct_t constructor copies

[redi at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=51183

--- Comment #17 from Jonathan Wakely <redi at gcc dot gnu.org> 2011-12-06 14:40:50 UTC ---
Does the new constructor need to be public?

[Bug libstdc++/51183] pair piecewise_construct_t constructor copies

[chris at bubblescope dot net]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=51183

--- Comment #18 from Chris Jefferson <chris at bubblescope dot net> 2011-12-06 14:41:19 UTC ---
2011-12-06  Chris Jefferson <chris@bubblescope.net>

    PR libstdc++/51183
    * include/std/tuple (pair::pair): Add two constructors which
    use delegating constructors
    (pair::__cons, pair::__do_cons): Remove unused functions.
    * include/std/stl_pair.h (pair::pair): Add declarations for
    constructors which use delegating constructors.
    (pair::__cons, pair::__do_cons): Remove unused declarations.
    * testsuite/20_util/pair/51183.cc: New.

[Bug libstdc++/51183] pair piecewise_construct_t constructor copies

[paolo.carlini at oracle dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=51183

--- Comment #19 from Paolo Carlini <paolo.carlini at oracle dot com> 2011-12-06 14:53:05 UTC ---
Nope, doesn't. I'm going to test and commit a version with the constructor
private. Thanks to both of you! By the way, it would be nice at some point to
actually analyze the assembly and the output of the passes and see which
difference by const-ref vs value makes.

[Bug libstdc++/51183] pair piecewise_construct_t constructor copies

[paolo at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=51183

--- Comment #20 from paolo at gcc dot gnu.org <paolo at gcc dot gnu.org> 2011-12-06 15:13:10 UTC ---
Author: paolo
Date: Tue Dec  6 15:13:04 2011
New Revision: 182054

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=182054
Log:
2011-12-06  Jonathan Wakely  <jwakely.gcc@gmail.com>
        Chris Jefferson  <chris@bubblescope.net>

    PR libstdc++/51183
    * include/std/stl_pair.h (pair<>::__cons, pair<>::__do_cons): Remove.
    (pair<>::pair(piecewise_construct_t, tuple<>, tuple<>): Only declare.
    (pair<>::pair(tuple<>&, tuple<>&, _Index_tuple<>, _Index_tuple<>)):
    Declare.
    * include/std/tuple (pair<>::__cons, pair<>::__do_cons): Remove.
    (pair<>::pair(tuple<>&, tuple<>&, _Index_tuple<>, _Index_tuple<>)):
    Define.
    (pair<>::pair(piecewise_construct_t, tuple<>, tuple<>): Define,
    delegating to the latter.
    * testsuite/20_util/pair/piecewise2.cc: New.

Added:
    trunk/libstdc++-v3/testsuite/20_util/pair/piecewise2.cc
Modified:
    trunk/libstdc++-v3/ChangeLog
    trunk/libstdc++-v3/include/bits/stl_pair.h
    trunk/libstdc++-v3/include/std/tuple

[Bug libstdc++/51183] pair piecewise_construct_t constructor copies

[paolo.carlini at oracle dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=51183

Paolo Carlini <paolo.carlini at oracle dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |RESOLVED
         Resolution|                            |FIXED

--- Comment #21 from Paolo Carlini <paolo.carlini at oracle dot com> 2011-12-06 15:27:20 UTC ---
Done.