Patchwork Value type of map need not be default copyable

login
register
mail settings
Submitter Ollie Wild
Date Aug. 3, 2012, 4:17 a.m.
Message ID <CAFOgFcTNNZSObWgAwSqWghH66ddkHsAMLaskmJFH6rPT=Upb3Q@mail.gmail.com>
Download mbox | patch
Permalink /patch/174875/
State New
Headers show

Comments

Ollie Wild - Aug. 3, 2012, 4:17 a.m.
Patch courtesy Richard Smith at Google:

Fix bug in the implementation of std::map's operator[]. Construct an
object of type value_type, rather than using std::make_pair, so as to
allow mapped_type to have an *explicit* copy constructor.

See [map.access] (23.4.4.3)/5 for the corresponding standardese.

Tested via bootstrap + test.

Okay for trunk?

Thanks,
Ollie


2012-08-02  Ollie Wild  <aaw@google.com>
	    Richard Smith  <richardsmith@google.com>

	* include/bits/stl_map.h (operator[](key_type&&)): Replace
	std::make_pair with value_type.
	* testsuite/23_containers/map/operators/2.cc: New test.
Paolo Carlini - Aug. 3, 2012, 9:39 a.m.
Hi,

On 08/03/2012 06:17 AM, Ollie Wild wrote:
> Patch courtesy Richard Smith at Google:
>
> Fix bug in the implementation of std::map's operator[]. Construct an
> object of type value_type, rather than using std::make_pair, so as to
> allow mapped_type to have an *explicit* copy constructor.
>
> See [map.access] (23.4.4.3)/5 for the corresponding standardese.
>
> Tested via bootstrap + test.
>
> Okay for trunk?
Ok, but, can you also double check and in case fix unordered_map too? 
Looks like we have the same issue, right?

Thanks!
Paolo.

PS: remember that all the library patches go to libstdc++@ too, and 
preferably please add something to the Subject about the library (like 
"[v3] ... ")
Ollie Wild - Aug. 3, 2012, 3:19 p.m.
On Fri, Aug 3, 2012 at 2:39 AM, Paolo Carlini <paolo.carlini@oracle.com> wrote:
>
> Ok, but, can you also double check and in case fix unordered_map too?
> Looks like we have the same issue, right?

Indeed, we do.  I'll send a separate patch for the unordered_map problem.

>
> Thanks!
> Paolo.
>
> PS: remember that all the library patches go to libstdc++@ too, and
> preferably please add something to the Subject about the library (like "[v3]
> ... ")

Thanks for the reminder.

Ollie
Paolo Carlini - Aug. 4, 2012, 11:23 a.m.
On 08/03/2012 05:19 PM, Ollie Wild wrote:
> On Fri, Aug 3, 2012 at 2:39 AM, Paolo Carlini <paolo.carlini@oracle.com> wrote:
>> Ok, but, can you also double check and in case fix unordered_map too?
>> Looks like we have the same issue, right?
> Indeed, we do.  I'll send a separate patch for the unordered_map problem.
But apparently something doesn't work together with the complex tangle 
of issues having to do with Core/1402 (aka c++/53733): the new testcase 
doesn't compile in mainline.

Thus, I'm reverting the patch for now. The we'll have to reanalyze 
whether the library specs should be further tweaked (beyond the 
container' bits of libstdc++/53657) in the light of Core/1402.

Paolo.
Paolo Carlini - Aug. 4, 2012, 11:54 a.m.
.. note anyway, that only the new testcase was failing, no regressions 
on pre existing testcases. Thus, it may well be that only the testcase 
had issues, whereas the code changes themselves (likewise those for 
unordered_map) are fine as they are, no changes needed elsewhere, neithe 
in the specs nor in our code. I didn't really further investigate so far 
(in any case we don't want unexpected fails in the testsuite)

Iw would be great if you could get into the details.

Thanks,
Paolo.
Marc Glisse - Aug. 4, 2012, 1:26 p.m.
On Sat, 4 Aug 2012, Paolo Carlini wrote:

> .. note anyway, that only the new testcase was failing, no regressions on pre 
> existing testcases.

What I am seeing is a different testcase (with the same name but in a 
different directory) failing, because:

   typedef std::pair<const rvalstruct,rvalstruct> V;
   static_assert(std::is_constructible<V, V&&>::value,"too bad");

and it makes sense, since you end up having to construct a rvalstruct from 
a rvalstruct const&&.

make_pair constructs a pair without const, from which a pair with const is 
constructible, though I am surprised it doesn't fail somewhere further. I 
don't know what the right solution is, maybe something emplace-like. In 
any case, make_pair is unlikely to be right.
Paolo Carlini - Aug. 4, 2012, 3:08 p.m.
On 08/04/2012 03:26 PM, Marc Glisse wrote:
> On Sat, 4 Aug 2012, Paolo Carlini wrote:
>
>> .. note anyway, that only the new testcase was failing, no 
>> regressions on pre existing testcases.
>
> What I am seeing is a different testcase (with the same name but in a 
> different directory) failing, because:
>
>   typedef std::pair<const rvalstruct,rvalstruct> V;
>   static_assert(std::is_constructible<V, V&&>::value,"too bad");
>
> and it makes sense, since you end up having to construct a rvalstruct 
> from a rvalstruct const&&.
Oops, you are right, sorry. To be clear, the testcase which was failing 
with the patch applied is (just check testresults, many examples) is:

   23_containers/map/element_access/2.cc

> make_pair constructs a pair without const, from which a pair with 
> const is constructible, though I am surprised it doesn't fail 
> somewhere further. I don't know what the right solution is, maybe 
> something emplace-like. In any case, make_pair is unlikely to be right.
I'm not sure to understand which specific testcase you are discussing, 
but for sure we don't want regressions. I agree that we should assume a 
priori that the standard is right, but correcting the make_pair should 
not lead to failures elsewhere (unless a proper analysis establishes 
that the existing testcases are wrong)

Paolo.
Marc Glisse - Aug. 4, 2012, 3:16 p.m.
On Sat, 4 Aug 2012, Paolo Carlini wrote:

> I'm not sure to understand which specific testcase you are discussing, but 
> for sure we don't want regressions. I agree that we should assume a priori 
> that the standard is right, but correcting the make_pair should not lead to 
> failures elsewhere (unless a proper analysis establishes that the existing 
> testcases are wrong)

Let's say it currently works by accident. What I believe is needed is to 
implement the missing emplace function, and then operator[] and others can 
be made to use it.
Paolo Carlini - Aug. 4, 2012, 3:19 p.m.
On 08/04/2012 05:16 PM, Marc Glisse wrote:
> On Sat, 4 Aug 2012, Paolo Carlini wrote:
>
>> I'm not sure to understand which specific testcase you are 
>> discussing, but for sure we don't want regressions. I agree that we 
>> should assume a priori that the standard is right, but correcting the 
>> make_pair should not lead to failures elsewhere (unless a proper 
>> analysis establishes that the existing testcases are wrong)
>
> Let's say it currently works by accident. What I believe is needed is 
> to implement the missing emplace function, and then operator[] and 
> others can be made to use it.
Are you really sure that emplace is involved? I'm not. The letter of the 
standard uses 'inserts'.

Paolo.
Marc Glisse - Aug. 4, 2012, 3:27 p.m.
On Sat, 4 Aug 2012, Paolo Carlini wrote:

> On 08/04/2012 05:16 PM, Marc Glisse wrote:
>> On Sat, 4 Aug 2012, Paolo Carlini wrote:
>> 
>>> I'm not sure to understand which specific testcase you are discussing, but 
>>> for sure we don't want regressions. I agree that we should assume a priori 
>>> that the standard is right, but correcting the make_pair should not lead 
>>> to failures elsewhere (unless a proper analysis establishes that the 
>>> existing testcases are wrong)
>> 
>> Let's say it currently works by accident. What I believe is needed is to 
>> implement the missing emplace function, and then operator[] and others can 
>> be made to use it.
> Are you really sure that emplace is involved? I'm not. The letter of the 
> standard uses 'inserts'.

The font indicates it is "english" insert, not "function" insert. In the 
testcase, value_type is not move constructible, so once you have an object 
of type value_type, you can't do anything with it.
Paolo Carlini - Aug. 4, 2012, 3:34 p.m.
On 08/04/2012 05:27 PM, Marc Glisse wrote:
> On Sat, 4 Aug 2012, Paolo Carlini wrote:
>
>> On 08/04/2012 05:16 PM, Marc Glisse wrote:
>>> On Sat, 4 Aug 2012, Paolo Carlini wrote:
>>>
>>>> I'm not sure to understand which specific testcase you are 
>>>> discussing, but for sure we don't want regressions. I agree that we 
>>>> should assume a priori that the standard is right, but correcting 
>>>> the make_pair should not lead to failures elsewhere (unless a 
>>>> proper analysis establishes that the existing testcases are wrong)
>>>
>>> Let's say it currently works by accident. What I believe is needed 
>>> is to implement the missing emplace function, and then operator[] 
>>> and others can be made to use it.
>> Are you really sure that emplace is involved? I'm not. The letter of 
>> the standard uses 'inserts'.
> The font indicates it is "english" insert, not "function" insert. In 
> the testcase, value_type is not move constructible, so once you have 
> an object of type value_type, you can't do anything with it.
First, I think we should add libstdc++ in CC.

Thus, I would recommend people working in this area to begin with 
unordered_map, because in that case emplace is already available, 
assuming that's really the point (and therefore reverting the patch was 
a good idea, luckily an existing testcase helped us)

At the same time an implementation of emplace is definitely welcome, in 
any case.

Paolo.

Patch

diff --git a/libstdc++-v3/include/bits/stl_map.h b/libstdc++-v3/include/bits/stl_map.h
index cfd478a..a3abdd4 100644
--- a/libstdc++-v3/include/bits/stl_map.h
+++ b/libstdc++-v3/include/bits/stl_map.h
@@ -475,7 +475,7 @@  _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
 	iterator __i = lower_bound(__k);
 	// __i->first is greater than or equivalent to __k.
 	if (__i == end() || key_comp()(__k, (*__i).first))
-          __i = insert(__i, std::make_pair(std::move(__k), mapped_type()));
+          __i = insert(__i, value_type(std::move(__k), mapped_type()));
 	return (*__i).second;
       }
 #endif
diff --git a/libstdc++-v3/testsuite/23_containers/map/operators/2.cc b/libstdc++-v3/testsuite/23_containers/map/operators/2.cc
new file mode 100644
index 0000000..ce633d7
--- /dev/null
+++ b/libstdc++-v3/testsuite/23_containers/map/operators/2.cc
@@ -0,0 +1,38 @@ 
+// Copyright (C) 2012
+// 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/>.
+
+// 23.4.4 template class map
+
+// This test verifies that the value type of a map need not be default
+// copyable.
+
+// { dg-do compile }
+// { dg-options "-std=gnu++11" }
+
+#include <map>
+
+struct Mapped {
+    Mapped();
+    explicit Mapped(const Mapped&);
+};
+
+Mapped & foo()
+{
+  std::map<int, Mapped> m;
+  return m[0];
+}