Tracking Surface for Athena MT. ATLASRECTS-4623

Merged Christos Anastopoulos requested to merge ATLAS-EGamma/athena:Surfaces_pass_I into master

The main purpose is to eventually ATLAS_CHECK_THREAD_SAFETY in Tracking/TrkDetDescr/TrkSurfaces

I will use as an example the Surface.h class .

Lazy Init

  • Move to cachedUniquePtr for the things that are lazily initialized. Examples :
CxxUtils::CachedUniquePtrT<Amg::Transform3D>  m_transform;     //!< Transform3D to orient surface w.r.t to global frame
CxxUtils::CachedUniquePtrT<Amg::Vector3D>     m_center;        //!< center position of the surface
CxxUtils::CachedUniquePtrT<Amg::Vector3D>     m_normal;        //!< normal vector of the surface

A method that instantiates a member at 1st use and then returns the existing instance (does not modify afterwards) and is logically const changes from:

inline const Amg::Vector3D& Surface::center() const
  {
    if (m_transform && !m_center) m_center = new Amg::Vector3D(m_transform->translation());
    if (m_center) return (*m_center);
.....

to :

    if(m_transform && !m_center){
          return *(m_center.set( 
                         std::make_unique<Amg::Vector3D>(m_transform->translation())
                        ));

    }
    if(m_center) return (*m_center);
......

const setters

  • This class (and her descendants) have certain const setter, things like
void setOwner(SurfaceOwner x) const

allowing modification of const object.

  • Unfortunatelly this feature is used/propagated in many places so will take a bit of time to migrate all depended code

  • In this case (although we could silence the warnings with atomic etc) unlike the above a setter can not be really be logically const!

  • So for now I try to provide this kind of way for client migration to const correctness:

       /** Set ownership  
       * Athena MT note : This should not be really used
       * as it  modifies a const object.
       * The non-const overload below is fine.
       */ 
      void setOwner  ATLAS_NOT_THREAD_SAFE (SurfaceOwner x) const 
      { const_cast<SurfaceOwner&> (m_owner) = x; }

      /* set Ownership */      
      void setOwner(SurfaceOwner x)  
      { m_owner = x; }

      /** return ownership */
      SurfaceOwner owner() const 
      { return m_owner; }

i.e

  • A. Provide non-const setter methods, as they should have been to start with.
  • B. Leave the const setters for now there as to not break too much code.
  • C. Mark the const setters explicitly as non-thread safe
  • D. The idea is to migrate the clients in a staged manner and then remove the non Thread safe ones... obviously new code using setter on const objects will get warnings from the checker.
  • E see ATLASRECTS-4957 for such a case

Client packages (e.g TrkGeometry)

  • Changes due to using CxxUtils::CachedUniquePtrT

  • They include methods like move ,resize which are marked as const , but at least some of them are effectively setters.

  • For now I added comments on those to motivate a bit of a closer look. One package at a time I guess .... this is just the 1st step...

Mentioning @ssnyder.

Edited by Christos Anastopoulos

Merge request reports