Skip to content

Commit

Permalink
ENH: Add direct SmartPointer conversions to match raw Pointers
Browse files Browse the repository at this point in the history 
Some expect conversions were not available with SmartPointers.

The patch adds direct conversion to SmartPointer<const T>
from SmartPointer<T> and direct conversions when the raw pointer is
convertible i.e. from derived to base classes. The prior
implementation relied on implicit conversion to raw pointers and
assignment from raw pointers to the smart pointer. These direct
conversions support move semantics, which can bypass "expensive"
atomic reference counting operations.

The comparison operators are now non-member functions to have
symmetric pointer comparisons.

Additional std::nullptr_t argumented functions were added to avoid
ambiguity created from SmartPointer's implicit conversion to raw
pointers.

Change-Id: I45e56547fbd22a08cef8350dd5f4d9383e1d3167
  • Loading branch information
@blowekamp
blowekamp committed Feb 16, 2018
1 parent 801dab4 commit f9442c9
Show file tree
Hide file tree
Showing 3 changed files with 464 additions and 49 deletions.
152 changes: 103 additions & 49 deletions Modules/Core/Common/include/itkSmartPointer.h
View file
Expand Up @@ -20,6 +20,7 @@

#include <iostream>
#include <utility>
#include <type_traits>
#include "itkConfigure.h"


Expand Down Expand Up @@ -51,22 +52,46 @@ class SmartPointer
public:
using ObjectType = TObjectType;

template<typename T> using EnableIfConvertible =
typename std::enable_if<std::is_convertible<T*, TObjectType*>::value>;

/** Constructor */
SmartPointer ()
{ m_Pointer = nullptr; }
constexpr SmartPointer () ITK_NOEXCEPT:
m_Pointer(nullptr)
{ }

/** Copy constructor */
SmartPointer (const SmartPointer< ObjectType > & p):
SmartPointer (const SmartPointer& p) ITK_NOEXCEPT:
m_Pointer(p.m_Pointer)
{ this->Register(); }

constexpr SmartPointer (std::nullptr_t p) ITK_NOEXCEPT:
m_Pointer(p)
{}

/** constructor with implicit conversion of pointer type */
template<typename T,
typename = typename EnableIfConvertible<T>::type>
SmartPointer(const SmartPointer<T> &p) ITK_NOEXCEPT:
m_Pointer(p.m_Pointer)
{
this->Register();
}

/** Move constructor */
SmartPointer (SmartPointer<ObjectType > &&p)
: m_Pointer(p.m_Pointer)
SmartPointer (SmartPointer<ObjectType > &&p) ITK_NOEXCEPT :
m_Pointer(p.m_Pointer)
{ p.m_Pointer = nullptr; }

/** move constructor with implicit conversion of pointer type */
template<typename T,
typename = typename EnableIfConvertible<T>::type >
SmartPointer( SmartPointer<T> &&p) ITK_NOEXCEPT:
m_Pointer(p.m_Pointer)
{ p.m_Pointer = nullptr; }

/** Constructor to pointer p */
SmartPointer (ObjectType *p):
SmartPointer (ObjectType *p) ITK_NOEXCEPT:
m_Pointer(p)
{ this->Register(); }

Expand All @@ -78,56 +103,40 @@ class SmartPointer
}

/** Overload operator -> */
ObjectType * operator->() const
ObjectType * operator->() const ITK_NOEXCEPT
{ return m_Pointer; }

ObjectType &operator*() const ITK_NOEXCEPT
{return *m_Pointer;}

explicit operator bool() const ITK_NOEXCEPT
{ return m_Pointer != nullptr; }

/** Return pointer to object. */
operator ObjectType *() const
{ return m_Pointer; }
operator ObjectType *() const ITK_NOEXCEPT
{ return m_Pointer; }

/** Test if the pointer is not NULL. */
bool IsNotNull() const
bool IsNotNull() const ITK_NOEXCEPT
{ return m_Pointer != nullptr; }

/** Test if the pointer is NULL. */
bool IsNull() const
bool IsNull() const ITK_NOEXCEPT
{ return m_Pointer == nullptr; }

/** Template comparison operators. */
template< typename TR >
bool operator==(TR r) const
{ return ( m_Pointer == static_cast< const ObjectType * >( r ) ); }

template< typename TR >
bool operator!=(TR r) const
{ return ( m_Pointer != static_cast< const ObjectType * >( r ) ); }

/** Access function to pointer. */
ObjectType * GetPointer() const
ObjectType * GetPointer() const ITK_NOEXCEPT
{ return m_Pointer; }

/** Comparison of pointers. Less than comparison. */
bool operator<(const SmartPointer & r) const
{ return (void *)m_Pointer < (void *)r.m_Pointer; }

/** Comparison of pointers. Greater than comparison. */
bool operator>(const SmartPointer & r) const
{ return (void *)m_Pointer > (void *)r.m_Pointer; }

/** Comparison of pointers. Less than or equal to comparison. */
bool operator<=(const SmartPointer & r) const
{ return (void *)m_Pointer <= (void *)r.m_Pointer; }

/** Comparison of pointers. Greater than or equal to comparison. */
bool operator>=(const SmartPointer & r) const
{ return (void *)m_Pointer >= (void *)r.m_Pointer; }

/** Overload operator assignment.
*
* This method is also implicitly used for move semantics.
* Additionally, it relies on constructors for additional conversion
* for pointer types.
*/
// cppcheck-suppress operatorEqVarError
SmartPointer & operator=(SmartPointer r)
SmartPointer & operator=(SmartPointer r) ITK_NOEXCEPT
{
// The Copy-Swap idiom is used, with the implicit copy from the
// value-based argument r (intentionally not reference). If a move
Expand All @@ -136,14 +145,12 @@ class SmartPointer
return *this;
}

/** Overload operator assignment. */
SmartPointer & operator=(ObjectType *r)
{
SmartPointer temp(r);
this->Swap(temp);

return *this;
}
SmartPointer &operator=(std::nullptr_t) ITK_NOEXCEPT
{
this->UnRegister();
this->m_Pointer = nullptr;
return *this;
}

/** Function to print object pointed to */
ObjectType * Print(std::ostream & os) const
Expand All @@ -161,13 +168,13 @@ class SmartPointer
}

#if ! defined ( ITK_LEGACY_REMOVE )
void swap(SmartPointer &other)
void swap(SmartPointer &other) ITK_NOEXCEPT
{
this->Swap(other);
}
#endif

void Swap(SmartPointer &other)
void Swap(SmartPointer &other) ITK_NOEXCEPT
{
ObjectType *tmp = this->m_Pointer;
this->m_Pointer = other.m_Pointer;
Expand All @@ -178,7 +185,10 @@ class SmartPointer
/** The pointer to the object referred to by this smart pointer. */
ObjectType *m_Pointer;

void Register()
template<typename T>
friend class SmartPointer;

void Register() ITK_NOEXCEPT
{
if ( m_Pointer ) { m_Pointer->Register(); }
}
Expand All @@ -189,6 +199,50 @@ class SmartPointer
}
};


/** Comparison of pointers. Equality comparison. */
template < class T, class TU >
bool operator==( const SmartPointer<T>& l, const SmartPointer<TU>& r ) ITK_NOEXCEPT
{ return ( l.GetPointer() == r.GetPointer() ); }
template < class T >
bool operator==( const SmartPointer<T>& l, std::nullptr_t ) ITK_NOEXCEPT
{ return ( l.GetPointer() == nullptr ); }
template < class T >
bool operator==( std::nullptr_t, const SmartPointer<T>& r ) ITK_NOEXCEPT
{ return ( nullptr == r.GetPointer() ); }

/** Comparison of pointers. Not equal comparison. */
template < class T, class TU >
bool operator!=( const SmartPointer<T>& l, const SmartPointer<TU>& r ) ITK_NOEXCEPT
{ return ( l.GetPointer() != r.GetPointer() ); }
template < class T >
bool operator!=( const SmartPointer<T>& l, std::nullptr_t ) ITK_NOEXCEPT
{ return ( l.GetPointer() != nullptr ); }
template < class T >
bool operator!=( std::nullptr_t, const SmartPointer<T>& r ) ITK_NOEXCEPT
{ return ( nullptr != r.GetPointer() ); }


/** Comparison of pointers. Less than comparison. */
template < class T, class TU >
bool operator<( const SmartPointer<T>& l, const SmartPointer<TU>& r ) ITK_NOEXCEPT
{ return ( l.GetPointer() < r.GetPointer() ); }

/** Comparison of pointers. Greater than comparison. */
template < class T, class TU >
bool operator>( const SmartPointer<T>& l, const SmartPointer<TU>& r ) ITK_NOEXCEPT
{ return ( l.GetPointer() > r.GetPointer() ); }

/** Comparison of pointers. Less than or equal to comparison. */
template < class T, class TU >
bool operator<=( const SmartPointer<T>& l, const SmartPointer<TU>& r ) ITK_NOEXCEPT
{ return ( l.GetPointer() <= r.GetPointer() ); }

/** Comparison of pointers. Greater than or equal to comparison. */
template < class T, class TU >
bool operator>=( const SmartPointer<T>& l, const SmartPointer<TU>& r ) ITK_NOEXCEPT
{ return ( l.GetPointer() >= r.GetPointer() ); }

template< typename T >
std::ostream & operator<<(std::ostream & os, SmartPointer< T > p)
{
Expand All @@ -197,7 +251,7 @@ std::ostream & operator<<(std::ostream & os, SmartPointer< T > p)
}

template<typename T>
inline void swap( SmartPointer<T> &a, SmartPointer<T> &b )
inline void swap( SmartPointer<T> &a, SmartPointer<T> &b ) ITK_NOEXCEPT
{
a.Swap(b);
}
Expand Down
6 changes: 6 additions & 0 deletions Modules/Core/Common/test/CMakeLists.txt
View file
Expand Up @@ -565,6 +565,12 @@ endif()
# try to compile it the compile fails.
# itk_add_test(NAME itkVectorMultiplyTest COMMAND ITKCommon2TestDriver itkVectorMultiplyTest)


set(ITKCommonGTests
itkSmartPointerGTest.cxx)

CreateGoogleTestDriver(ITKCommon "${ITKCommon-Test_LIBRARIES}" "${ITKCommonGTests}")

itk_python_add_test(NAME itkMetaDataDictionaryPythonTest COMMAND itkMetaDataDictionaryTest.py)
itk_python_add_test(NAME itkDirectoryPythonTest COMMAND itkDirectoryTest.py)
itk_python_expression_add_test(NAME itkObjectPythonTest EXPRESSION "itkObject = itk.Object.New()")
Expand Down

6 comments on commit f9442c9

@skn123
Copy link

@skn123 skn123 commented on f9442c9 Feb 17, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This commit is breaking builds.

@blowekamp
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you have a specific failure to report? Or a proposed fix?

@skn123
Copy link

@skn123 skn123 commented on f9442c9 Feb 18, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is a code that is failing. I have something similar when building SimpleITK. Is there a new convention that we need to follow here?
conversion from 'long' to 'spin::RGBImageType::Pointer' (aka 'SmartPointer<itk::Image<itk::RGBPixel<unsigned char>, 2> >') is ambiguous spin::RGBImageType::Pointer fg = NULL; ^ ~~~~ /usr/local/include/ITK-5.0/itkSmartPointer.h:68:13: note: candidate constructor constexpr SmartPointer (std::nullptr_t p) noexcept: ^ /usr/local/include/ITK-5.0/itkSmartPointer.h:94:3: note: candidate constructor SmartPointer (ObjectType *p) noexcept: ^

@skn123
Copy link

@skn123 skn123 commented on f9442c9 Feb 18, 2018 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or, maybe you should update this wiki
https://itk.org/Wiki/ITK/Tutorials/DOs_and_DONTs
`` Do not call Delete() in an ITK smart pointer.
If you want to destroy a smart pointer object itksmartp, do:

      itksmartp = NULL;

the ITK smart pointer will determine whether the object should be "deleted".
``

Compiler is clang 4.0.1, Ubuntu 17.10 64Bit

@skn123
Copy link

@skn123 skn123 commented on f9442c9 Feb 18, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at the API; the following code compiled.
spin::RGBImageType::Pointer fg = spin::RGBImageType::Pointer();

@blowekamp
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please move the conversion to the ITK discussion group:
https://discourse.itk.org

Please keep in mind that ITK 5.0 is under active development of a major revision and some instability is expected.

Please sign in to comment.