Logo Questions Linux Laravel Mysql Ubuntu Git Menu
 

Is there a bug in GCC 4.7.2's implementation of shared_ptr's (templated) assignment operator?

My question concerns the implementation of shared_ptr's assignment operator template in GCC 4.7.2, which I suspect to contain a bug.

PREMISE 1: C++11 STANDARD

Here is the signature of the assignment operator template I am talking about:

template<class Y> shared_ptr& operator=(const shared_ptr<Y>& r) noexcept;

From the C++11 Standard (20.7.2.2.3):

"Equivalent to shared_ptr(r).swap(*this)."

In other words, the assignment operator template is defined in terms of constructor template. The signature of the constructor template is as follows:

template<class Y> shared_ptr(const shared_ptr<Y>& r) noexcept;

From the C++11 Standard (20.7.2.2.1):

"Requires: The [...] constructor shall not participate in the overload resolution unless Y* is implicitly convertible to T*."

PREMISE 2: GCC 4.7.2's IMPLEMENTATION:

Now GCC 4.7.2's implementation of the constructor template seems correct to me (std::__shared_ptr is a base class of std::shared_ptr):

template<typename _Tp1, typename = 
typename std::enable_if<std::is_convertible<_Tp1*, _Tp*>::value>::type>
__shared_ptr(__shared_ptr<_Tp1, _Lp>&& __r) noexcept
    : 
    _M_ptr(__r._M_ptr), 
    _M_refcount()
{
    _M_refcount._M_swap(__r._M_refcount);
    __r._M_ptr = 0;
}

However, GCC 4.7.2's implementation of the assignment operator template is the following:

template<typename _Tp1>
__shared_ptr& operator=(const __shared_ptr<_Tp1, _Lp>& __r) noexcept
{
    _M_ptr = __r._M_ptr;
    _M_refcount = __r._M_refcount; // __shared_count::op= doesn't throw
    return *this;
}

What strikes me is that this operation is not defined in terms of the constructor template, nor of swap(). In particular, the plain assignment _M_ptr = __r._M_ptr does not yield the same result as when the types _Tp1* and _Tp* are explicitly checked for convertibility through std::is_convertible (which can be specialized).

PREMISE 3: VC10 IMPLEMENTATION

I noticed that VC10 does have a more conforming implementation in this respect, which I consider to be correct and behaves as I expected in my test cases (while GCC's doesn't):

template<class _Ty2>
_Myt& operator=(const shared_ptr<_Ty2>& _Right)
{
    // assign shared ownership of resource owned by _Right
    shared_ptr(_Right).swap(*this);
    return (*this);
}

QUESTION:

Is there indeed a bug in GCC 4.7.2's implementation of shared_ptr? I could not find any bug report for this issue.

POST SCRIPTUM:

If you wish to ask me what my test cases are, why do I care about this seemingly unimportant detail and why do I seem to imply that I need to specialize std::is_convertible, please do so in chat. It is a long story and there's no way of summing it up without being misunderstood (with all of its unpleasant consequences). Thank you in advance.

like image 330
Andy Prowl Avatar asked Jan 08 '13 18:01

Andy Prowl


2 Answers

What strikes me is that this operation is not defined in terms of the constructor template, nor of swap().

It doesn't need to be, it only needs to behave as if it was defined in those terms.

In particular, the plain assignment _M_ptr = __r._M_ptr does not yield the same result as when the types _Tp1* and _Tp* are explicitly checked for convertibility through std::is_convertible (which can be specialized).

I disagree: [meta.type.synop]/1 The behavior of a program that adds specializations for any of the class templates defined in this subclause is undefined unless otherwise specified.

So you can't change the meaning of is_convertible<Y*, T*> and if Y* is convertible to T* then the assignment will work and since both assignments (of the pointer and the refcount object) are noexcept the end result is equivalent to the swap. If the pointers are not convertible then the assignment will fail to compile, but so would shared_ptr(r).swap(*this), so it's still equivalent.

If I'm wrong then please file a bug report and I'll fix it, but I don't think a conforming program can detect the difference between the libstdc++ implementation and the requirements of the standard. That said, I wouldn't have any objections to changing it to be implemented in terms of swap. The current implementation came straight from the shared_ptr in Boost 1.32, I don't know if Boost still does it the same way or if it uses shared_ptr(r).swap(*this) now.

[Full disclosure, I'm a libstdc++ maintainer and mostly responsible for the shared_ptr code, which was originally kindly donated by the authors of boost::shared_ptr and then mutiliated by me since then.]

like image 64
Jonathan Wakely Avatar answered Oct 06 '22 20:10

Jonathan Wakely


The implementation in GCC conforms with the requirements in the standard. When the standard defines that the behavior of one function is equivalent to a different set of functions it means that the effect of the former is equivalent to the effect of the latter functions as defined in the standard (rather than as implemented).

The standard does not require the use of std::is_convertible for that constructor. It requires SFINAE for the constructor, but it does not require SFINAE for the assignment operator. The requirement that the types are convertible are placed on the program, not on the implementation of the std::shared_ptr and it is your responsibility. If the types passed in are not convertible, then it is a bug in your program. If they are then the implementation must accept the code even if you wanted to disable the use by specializing the is_convertible template.

Then again, specializing is_convertible to limit the conversions of pointers is undefined behavior, as you are changing the semantics of the base template, which is explicitly disallowed in the standard.

This leads to the original question that you don't want to answer: what is the use case that made you think on this solution. Or said otherwise, why do people keep on asking about solutions rather than about the real problems they want solved?

like image 41
David Rodríguez - dribeas Avatar answered Oct 06 '22 20:10

David Rodríguez - dribeas