[Ipopt-tickets] [Ipopt] #162: unsave SmartPtr copy semantics, meaningless comparisons, missing conversions
Ipopt
coin-trac at coin-or.org
Wed Jun 22 14:32:43 EDT 2011
#162: unsave SmartPtr copy semantics, meaningless comparisons, missing
conversions
---------------------+------------------------------------------------------
Reporter: guest | Owner: ipopt-team
Type: defect | Status: new
Priority: normal | Component: Ipopt
Version: 3.9 | Severity: normal
Keywords: SmartPtr |
---------------------+------------------------------------------------------
Hi,
there seem to exist several issues with the implementation of SmartPtr
that make it unsafe to use in certain contexts.
I have not actually experienced any bug myself, this is solely derived
from studying the source code, triggered by this post:
http://www.c-plusplus.de/forum/288762 (in german).
---------------------------------------------------------------------------------------------------------------------------------
1. SmartPtr does not handle self assignment properly if it is the sole
referent to an object.
Consider
struct foo : ReferencedObject { ... };
SmartPtr<foo> p = new foo;
p = p; // (1)
p = GetRawPointer( p ); // (2)
Both (1) and (2) will cause p to be a null pointer afterwards. This is
because in SmartPtr<T>::SetFromSmartPtr_ the old pointer is released
before the reference count for the right hand side is incremented.
This makes SmartPtr incompatible with standard containers and mutating
standard algorithms.
The fix would be straight forward by changing the order:
template <class T>
SmartPtr<T>& SmartPtr<T>::SetFromRawPtr_(T* rhs)
{
#ifdef IP_DEBUG_SMARTPTR
DBG_START_METH(
"SmartPtr<T>& SmartPtr<T>::SetFromRawPtr_(T* rhs)",
dbg_smartptr_verbosity);
#endif
if (rhs != 0)
rhs->AddRef(this);
// Release any old pointer
ReleasePointer_();
ptr_ = rhs;
return *this;
}
SmartPtr<T>::ReleasePointer_ must be patched to not modify ptr_ - since
the only other function that calles it is the destructor, this is not a
problem:
template <class T>
void SmartPtr<T>::ReleasePointer_()
{
#ifdef IP_DEBUG_SMARTPTR
DBG_START_METH(
"void SmartPtr<T>::ReleasePointer()",
dbg_smartptr_verbosity);
#endif
if (ptr_) {
ptr_->ReleaseRef(this);
if (ptr_->ReferenceCount() == 0) {
delete ptr_;
}
}
}
An explicit test for self assignment could be used but is not the best
choice since it only conceals the logic error and it might mean an
unnecessary performance hit since self assignment ist quite rare but the
test would have to be done during each assignment.
---------------------------------------------------------------------------------------------------------------------------------
2. The comparison done in
template <class U1, class U2>
bool ComparePointers(const U1* lhs, const U2* rhs)
either does not work as intended or is meaningless if different types are
involved, this affects the overloaded operators == and !=
consider the current code in trunk:
561 template <class U1, class U2>
562 bool ComparePointers(const U1* lhs, const U2* rhs)
563 {
564 #ifdef IP_DEBUG_SMARTPTR
565 DBG_START_FUN(
566 "bool ComparePtrs(const U1* lhs, const U2* rhs)",
567 dbg_smartptr_verbosity);
568 #endif
569
570 if (lhs == rhs) {
571 return true;
572 }
573
574 // Even if lhs and rhs point to the same object
575 // with different interfaces U1 and U2, we cannot guarantee
that
576 // the value of the pointers will be equivalent. We can
577 // guarantee this if we convert to void*
578 const void* v_lhs = static_cast<const void*>(lhs);
579 const void* v_rhs = static_cast<const void*>(rhs);
580 if (v_lhs == v_rhs) {
581 return true;
582 }
583
584 // They must not be the same
585 return false;
586 }
line 570 requires the template arguments to be compatible, that is one of
them must be convertible to the other. Specifically both are of the same
type (safe for const qualification), one of them is (const) void or one is
a public and unambigous base class of the other.
In all these cases the comparision in line 580 is meaningless, since it
will never yield any result that differs from that in line 570.
If line 580 is intended to handle the case where there existist multiple
interfaces to the same object that are not derived from each other (i.e.
multiple inheritance) it will not work as intended, because the conversion
to const void* by means of a static_cast will not result in pointers to
the complete objects. And line 570 would prevent the compiler to
instantiate the function for such a combination of template parameters in
the first place.
A conversion to a pointer that points to the most derived object can be
done by means of dynamic_cast, thus:
template <class U1, class U2>
bool ComparePointers(const U1* lhs, const U2* rhs)
{
#ifdef IP_DEBUG_SMARTPTR
DBG_START_FUN(
"bool ComparePtrs(const U1* lhs, const U2* rhs)",
dbg_smartptr_verbosity);
#endif
return dynamic_cast<const void*>(lhs) == dynamic_cast<const
void*>(rhs);
}
This requires lhs and rhs to be pointers to polymorphic types which is not
a problem since pointees must be derived from ReferencedObject.
Using dynamic_cast always might result in inacceptable performance since
most comparisions are probably using compatible types, therefore several
overloads could be used, for example:
template <typename T>
struct identity
{
typedef T type;
};
// the special case if both arguments are of the same type safe for const
qualification
template <typename T>
bool ComparePointers(const T* lhs, const T* rhs)
{
return lhs == rhs;
}
// called if the right hand side is convertible to the type of the left
hand side
template <typename T>
bool ComparePointers(const T* lhs, typename identity<const T>::type* rhs)
{
return ComparePointers(lhs, rhs);
}
// called if the left hand side is convertible to the type of the right
hand side
template <typename T>
bool ComparePointers(typename identity<const T>::type* lhs, const T* rhs)
{
return ComparePointers(lhs, rhs);
}
// the general case
template <typename T, typename U>
bool ComparePointers(T* lhs, U* rhs)
{
return dynamic_cast<const void*>(lhs) == dynamic_cast<const
void*>(rhs);
}
There is a simpler option by using the fact that all types used to
instantiate a SmartPtr must be derived from ReferencedObject:
template <typename T, typename U>
bool ComparePointers(T* lhs, U* rhs)
{
const ReferencedObject* lhs_ = lhs;
const ReferencedObject* rhs_ = rhs;
return lhs_ == rhs_;
}
No other overloads are needed in this case.
It should be made explicit in the documentation that only one
ReferencedObject may exist for every most derived object, so multiple
inheritance requires virtual inheritance.
---------------------------------------------------------------------------------------------------------------------------------
3. SmartPtr is missing implicit conversions to SmartPtr to bases. This is
inconvenient since it forces user code to either only use SmartPtr to base
classes and explicit casts, or rely on the intrusive nature of SmartPtr
(which should be an implementation detail).
template <class T>
template <class U>
SmartPtr<T>::SmartPtr(const SmartPtr<U>& copy)
:
ptr_(0)
{
(void) SetFromRawPtr_(GetRawPtr(copy));
}
template <class T>
template <class U>
SmartPtr<T>& SmartPtr<T>::operator=(const SmartPtr<U>& rhs)
{
return SetFromRawPtr_(GetRawPtr(copy));
}
should be provided.
explicit conversion functions that model static_cast and dynamic_cast
might be convenient as there exists already the member function ConstPtr:
template <class T, class U>
SmartPtr<T> StaticPtr(const SmartPtr<U>& smart_ptr)
{
return static_cast<T*>(GetRawPtr(smart_ptr));
}
template <class T, class U>
SmartPtr<T> DynamicPtr(const SmartPtr<U>& smart_ptr)
{
return dynamic_cast<T*>(GetRawPtr(smart_ptr));
}
---------------------------------------------------------------------------------------------------------------------------------
4. It may be convenient to additionally explicitily define swap, and
overloaded relational operators in order to make SmartPtr usable with
associative containers like std::set (it probably does not really make
sense to define ordering for different SmartPtr types, but it could be
done by relying on the unique ReferencedObject subobject):
// as member function
template <class T>
void SmartPtr<T>::swap(SmartPtr<T>& other)
{
using std::swap;
swap(ptr_, other.ptr_);
}
// as free function
template <class T>
void swap(SmartPtr<T>& a, SmartPtr<T>& b)
{
a.swap(b);
}
template <class T>
bool operator<(const SmartPtr<T>& lhs, const SmartPtr<T>& rhs)
{
return std::less<T*>()(lhs.ptr_, rhs.ptr_);
}
template <class T> bool operator> (const SmartPtr<T>& lhs, const
SmartPtr<T>& rhs) { return rhs < lhs; }
template <class T> bool operator<=(const SmartPtr<T>& lhs, const
SmartPtr<T>& rhs) { return !( rhs < lhs ); }
template <class T> bool operator>=(const SmartPtr<T>& lhs, const
SmartPtr<T>& rhs) { return !( lhs < rhs ); }
--
Ticket URL: <https://projects.coin-or.org/Ipopt/ticket/162>
Ipopt <http://projects.coin-or.org/Ipopt>
Interior-point optimizer for nonlinear programs.
More information about the Ipopt-tickets
mailing list