[prev in list] [next in list] [prev in thread] [next in thread] 

List:       cfe-dev
Subject:    Re: [cfe-dev] libc++ std::map self-assignment bug
From:       Howard Hinnant <hhinnant () apple ! com>
Date:       2013-11-29 16:42:33
Message-ID: 32A64891-D1B3-4D23-97A8-3A8B6AAE6841 () apple ! com
[Download RAW message or body]


On Nov 29, 2013, at 2:50 AM, Kal <b17c0de@gmail.com> wrote:

> Am 29.11.13 04:17, schrieb Howard Hinnant:
> > On Nov 28, 2013, at 9:58 PM, Karen Shaeffer <shaeffer@neuralscape.com> wrote:
> > 
> > > On Fri, Nov 29, 2013 at 02:19:44AM +0100, Kal wrote:
> > > > Hi Howard, etc,
> > > > 
> > > > libc++3.4 (rc1/trunk) std::map doesn't support self-assignment for std <
> > > > c++11. The relevant code is:
> > > > 
> > > > _LIBCPP_INLINE_VISIBILITY
> > > > map& operator=(const map& __m)
> > > > {
> > > > #if __cplusplus >= 201103L
> > > > __tree_ = __m.__tree_;
> > > > #else
> > > > __tree_.clear();
> > > > __tree_.value_comp() = __m.__tree_.value_comp();
> > > > __tree_.__copy_assign_alloc(__m.__tree_);
> > > > insert(__m.begin(), __m.end());
> > > > #endif
> > > > return *this;
> > > > }
> > > > 
> > > > Maybe should be like:
> > > > 
> > > > _LIBCPP_INLINE_VISIBILITY
> > > > map& operator=(const map& __m)
> > > > {
> > > > #if __cplusplus >= 201103L
> > > > __tree_ = __m.__tree_;
> > > > #else
> > > > if (this != &__m) {
> > > > __tree_.clear();
> > > > __tree_.value_comp() = __m.__tree_.value_comp();
> > > > __tree_.__copy_assign_alloc(__m.__tree_);
> > > > insert(__m.begin(), __m.end());
> > > > }
> > > > #endif
> > > > return *this;
> > > > }
> > > > 
> > > > I see the same issue with unordered_map& operator=(const unordered_map&
> > > > __u).
> > > > 
> > > > Thanks!
> > > > Kal
> > > --- end quoted text ---
> > > 
> > > Hi Kal,
> > > I don't speak for Howard or anyone else. But my understanding is the stl \
> > > library generally doesn't perform checks like that with the goal of best \
> > > possible performance. I see a lot of code in stdlibc++ just like that.
> > Actually this does look like a bug to me (for  __cplusplus < 201103L).  I think \
> > Kal has the correct fix.  A post-condition of copy assignment is that both lhs \
> > and rhs should be equivalent to the previous value of rhs.  For move assignment \
> > this is not the case.  But this is copy assignment. 
> > Howard
> > 
> Hi Howard,
> Thanks for the reply. Do you think a fix for this will be committed for
> the 3.4 release?

I don't know.

> 
> Also could you explain why the special check for __cplusplus < 201103L
> is necessary at all. Both branches are doing almost the same thing
> (looking into the implementation of __tree). Is there some subtle
> difference in the standard that requires a different implementation here?

C++11 introduces the possibility of recycling nodes of [multi]map under the copy \
assignment operator by pushing the requirements on [multi]map::value_type down to the \
key_type and mapped_type (23.2.4 [associative.reqmts]/p7).  To actually implement \
this optimization I used new C++11 language features of union, which do not compile \
when -std=c++03.

In C++11, using libc++, if two equal sized map<int, int> are assigned, there are zero \
trips to the heap.  The nodes in the lhs are assigned new values, and rebalancing \
takes place as necessary.  In C++03, the lhs is first clear()'d, and then one does an \
insert for every value in the rhs.

Same design (and bug as you note) in unordered_[multi]map.

Howard

_______________________________________________
cfe-dev mailing list
cfe-dev@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev


[prev in list] [next in list] [prev in thread] [next in thread]