Bug 51183 - pair piecewise_construct_t constructor copies
Summary: pair piecewise_construct_t constructor copies
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: libstdc++ (show other bugs)
Version: 4.7.0
: P3 normal
Target Milestone: 4.7.0
Assignee: Paolo Carlini
URL:
Keywords:
Depends on: 43674
Blocks:
  Show dependency treegraph
 
Reported: 2011-11-17 02:28 UTC by Marc Glisse
Modified: 2011-12-06 15:27 UTC (History)
2 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2011-11-17 00:00:00


Attachments
Tested patch (1.35 KB, patch)
2011-12-06 10:31 UTC, Paolo Carlini
Details | Diff
Piecewise patch (1.46 KB, patch)
2011-12-06 14:25 UTC, Chris Jefferson
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Marc Glisse 2011-11-17 02:28:51 UTC
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.
Comment 1 Chris Jefferson 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.
Comment 2 Jonathan Wakely 2011-11-17 09:30:41 UTC
nope, they haven't
Comment 3 Jonathan Wakely 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
Comment 4 Marc Glisse 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.
Comment 5 Paolo Carlini 2011-12-05 15:54:05 UTC
Chris, delegating constructors are just in, a patch would be welcome...
Comment 6 Jonathan Wakely 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))...)
        { }
Comment 7 Jonathan Wakely 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
Comment 8 Paolo Carlini 2011-12-05 16:12:35 UTC
I suspected that. Thus, Jon, if you like, just test and commit! ;)
Comment 9 Chris Jefferson 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.
Comment 10 Chris Jefferson 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.
Comment 11 Jonathan Wakely 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!
Comment 12 Paolo Carlini 2011-12-06 09:55:45 UTC
I'm taking care of this.
Comment 13 Chris Jefferson 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?
Comment 14 Paolo Carlini 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!
Comment 15 Paolo Carlini 2011-12-06 10:31:30 UTC
Created attachment 26005 [details]
Tested patch
Comment 16 Chris Jefferson 2011-12-06 14:25:24 UTC
Created attachment 26006 [details]
Piecewise patch

Patch to make piecewise_construct work properly on std::pair.
Comment 17 Jonathan Wakely 2011-12-06 14:40:50 UTC
Does the new constructor need to be public?
Comment 18 Chris Jefferson 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.
Comment 19 Paolo Carlini 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.
Comment 20 paolo@gcc.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
Comment 21 Paolo Carlini 2011-12-06 15:27:20 UTC
Done.