Skip to content

ATLASRECTS-4956 Migrate SharedObject to be a type alias to std::shared_ptr

SharedObject --> std::shared_ptr

The SharedObject among other things had an unsafe reference count.

In reality looks like a kind C++98 implementation of shared (ref counted) pointer.

This MR makes SharedObject to be an alias for shared_ptr.

Trivial changes for clients

Most of the changes so far on the clients are

  • Mainly foo->getPtr() --> foo->get() (seem like ~100)
  • A few return foo->getRef() --> return *(foo->get())

No Delete Option of SharedObject (the tricky bit)

The SharedObject class had a "not delete" option

//     given bool = true means object gets never deleted */
      SharedObject(T* tobj, bool ndel=false) :

The option ndel was set to true in 6-7 places. It signifies that you have a ptr that will be deleted from somewhere else, but still you want to construct a shared object with it . The reason are beyond this MR.

Obviously there is not std::shared-ptr(T* ptr, bool). there is shared_ptr( std::nullptr_t ptr, Deleter d )

So in this case the ndel=true has been replaced
with a shared_ptr with no-op deleter ( a nodelete deleter if it makes any sense) to try and retain the no delete semantics...

e.g

Trk::SharedObject<const Trk::TrackingVolume>(sVol, true)

which was not supposed to delete its element (due to the ndel=true) looks like

Trk::SharedObject<const Trk::TrackingVolume>(sVol, [](const Trk::TrackingVolume*){})

while

Trk::SharedObject<const Trk::TrackingVolume>(sVol,false)

which was the default SharedObject behaviour trivially becomes:

Trk::SharedObject<const Trk::TrackingVolume>(sVol)

One could imagine in the future that one would like some people closer to each code to take a critical look at the ownership relations that required nodel=true.....

Further clarification on the ndel replacement

For starters it feels like replacing a hack with a hack. So perhaps we should really understand what we want to do here long term look last phrase above.

Now, for some reason let's assume you have a ptr, and you want to pass it to a shared_ptr while retaining ownership (for some reason) of when it will be deleted. Similar to the ndel option of the SharedObject.

A simple example (to try at home) would be

#include <memory>

int main(){

  double* a = new double(10.90);
  //Now we want to both pass it in shared_ptr
  //and retain ownership. For some reason we 
  // want this... i.e ndel
 
  //way to achieve it
  std::shared_ptr<double> aShared=std::shared_ptr<double>(a,[](double*){});

  //segmantation from double delete 
  //std::shared_ptr<double> aShared=std::shared_ptr<double>(a);

  delete a;

  return 0;
}

Wrapping option

Another option would have been to wrap a shared_ptr inside this class. In principle an alias should make migrating and communicating with shared_ptr easier for the future, so was deemed to be preferred in the JIRA discussion.

Such a direct wrapper needing no client modification, would naively look like this.

class SharedObject{
public:
   SharedObject():m_ptr{}{
   }
   
   SharedObject(T* ptr, bool nodel=false){
      if(nodel==false){
        m_ptr=std::shared_ptr<T>(ptr);
      }else{
          m_ptr=std::shared_ptr<T>(ptr,[](T*){});
      }
   }

   SharedObject(const SharedObject& so): m_ptr{so.m_ptr}{
   }

  SharedObject& operator=(const SharedObject& so){
    if(this!=&so){
    this->m_ptr=so.m_ptr;
    }
    return *this;
    }
 T& operator*() const { return (*m_ptr);}  

  T* operator->() const { return m_ptr; }

 T& getRef() const { return (*m_ptr); }
       
 T* getPtr() const { return m_ptr; }

  private:
   std::shared_ptr<T> m_ptr;

};

The replacement in the client code maps closely to it. As in some sense the code had to be put into place at the client side than be "wrapped"

Mentioning @ssnyder

Edited by Christos Anastopoulos

Merge request reports

Loading