[RF] Improve implementation of RooAbsArg::redirectServers()

roofit/roofitcore/inc/RooAbsArg.h

@@ -266,7 +266,9 @@ class RooAbsArg : public TNamed, public RooPrintable {
 
   void addServer(RooAbsArg& server, bool valueProp=true, bool shapeProp=false, std::size_t refCount = 1);
   void addServerList(RooAbsCollection& serverList, bool valueProp=true, bool shapeProp=false) ;
-  void replaceServer(RooAbsArg& oldServer, RooAbsArg& newServer, bool valueProp, bool shapeProp) ;
+  void
+  R__SUGGEST_ALTERNATIVE("This interface is unsafe! Use RooAbsArg::redirectServers()")
+  replaceServer(RooAbsArg& oldServer, RooAbsArg& newServer, bool valueProp, bool shapeProp) ;
   void changeServer(RooAbsArg& server, bool valueProp, bool shapeProp) ;
   void removeServer(RooAbsArg& server, bool force=false) ;
   RooAbsArg *findNewServer(const RooAbsCollection &newSet, bool nameChange) const;

roofit/roofitcore/inc/RooSTLRefCountList.h

@@ -57,6 +57,9 @@ class RooSTLRefCountList {
     ///Add an object or increase refCount if it is already present. Only compares
     ///pointers to check for existing objects
     void Add(T * obj, std::size_t initialCount = 1) {
+      // Nothing to add because `refCount` would be zero.
+      if(initialCount == 0) return;
+
       auto foundItem = findByPointer(obj);
 
       if (foundItem != _storage.end()) {
@@ -191,12 +194,18 @@ class RooSTLRefCountList {
     ///Decrease ref count of given object. Shrink list if ref count reaches 0.
     ///\param obj Decrease ref count of given object. Compare by pointer.
     ///\param force If true, remove irrespective of ref count.
-    void Remove(const T * obj, bool force = false) {
+    ///Returns by how much the `refCount` for the element to be removed was
+    ///decreased (zero if nothing was removed). If `force == false`, it can
+    ///only be zero or one, if `force == true`, it can be the full `refCount`
+    ///for that element.
+    int Remove(const T * obj, bool force = false) {
       auto item = findByPointer(obj);
 
       if (item != _storage.end()) {
         const std::size_t pos = item - _storage.begin();
 
+        const UInt_t origRefCount = _refCount[pos];
+
         if (force || --_refCount[pos] == 0) {
           //gcc4.x doesn't know how to erase at the position of a const_iterator
           //Therefore, erase at begin + pos instead of 'item'
@@ -211,8 +220,31 @@ class RooSTLRefCountList {
             // _storage at the beginning of Remove().
             _orderedStorage.erase(std::find(_orderedStorage.begin(), _orderedStorage.end(), obj));
           }
+          return origRefCount;
         }
+        return 1;
       }
+
+      return 0;
+    }
+
+
+    ///Replace an element with a new value, keeping the same `refCount`. Will
+    ///return the `refCount` for that element if the replacement succeeded,
+    ///otherwise returns zero in case the `oldObj` could not be found in the
+    ///collection.
+    int Replace(const T * oldObj, T * newObj) {
+      auto item = findByPointer(oldObj);
+
+      if (item != _storage.end()) {
+        const std::size_t pos = item - _storage.begin();
+        _storage[pos] = newObj;
+        // The content has changed, so the ordered-by-name storage was invalidated.
+        _orderedStorage.clear();
+        return _refCount[pos];
+      }
+
+      return 0;
     }
 
 

roofit/roofitcore/src/RooAbsArg.cxx

@@ -422,12 +422,34 @@ void RooAbsArg::removeServer(RooAbsArg& server, bool force)
 
 
 ////////////////////////////////////////////////////////////////////////////////
-/// Replace 'oldServer' with 'newServer'
+/// Replace 'oldServer' with 'newServer', specifying whether the new server has
+/// value or shape server properties.
+///
+/// \warning This function should not be used! This method is quite unsafe for
+/// many reasons. For once, the new server will be put at the end of the server
+/// list, no matter the position of the original server. This might mess up
+/// code that expects the servers to be in a certain order. Furthermore, the
+/// proxy objects corresponding to the server are not updated, leaving the
+/// object in an invalid state where the servers are out of sync with the
+/// proxies. This can have very bad consequences. Finally, by having to
+/// manually specify the value and shape server properties, it is very easy to
+/// get them wrong.
+///
+/// If you want to safely replace a server, you should use
+/// RooAbsArg::redirectServers(), which replaces the server in-place at the
+/// same position of the server list, keeps the same value and shape server
+/// properties, and also updates the corresponding proxies.
 
 void RooAbsArg::replaceServer(RooAbsArg& oldServer, RooAbsArg& newServer, bool propValue, bool propShape)
 {
+  coutW(LinkStateMgmt) << "replaceServer()"
+      << " is unsafe, because the server list will be out of sync with the proxy objects!"
+      << " If you want to safely replace a server, use RooAbsArg::redirectServers()."
+      << " See the docs to replaceServers() for more info." << std::endl;
+
   Int_t count = _serverList.refCount(&oldServer);
   removeServer(oldServer, true);
+
   addServer(newServer, propValue, propShape, count);
 }
 
@@ -1007,50 +1029,29 @@ bool RooAbsArg::redirectServers(const RooAbsCollection& newSetOrig, bool mustRep
   if (newSetOrig.empty() || (newSetOrig.size() == 1 && newSetOrig[0] == this)) return false ;
 
   // Strip any non-matching removal nodes from newSetOrig
-  RooAbsCollection* newSet ;
+  std::unique_ptr<RooArgSet> newSetOwned;
+  RooAbsCollection const* newSet = &newSetOrig;
 
   if (nameChange) {
-    newSet = new RooArgSet ;
-    for (auto arg : newSetOrig) {
+    newSetOwned = std::make_unique<RooArgSet>();
+    for (auto arg : *newSet) {
 
       if (string("REMOVAL_DUMMY")==arg->GetName()) {
 
         if (arg->getAttribute("REMOVE_ALL")) {
-          newSet->add(*arg) ;
+          newSetOwned->add(*arg) ;
         } else if (arg->getAttribute(Form("REMOVE_FROM_%s",getStringAttribute("ORIGNAME")))) {
-          newSet->add(*arg) ;
+          newSetOwned->add(*arg) ;
         }
       } else {
-        newSet->add(*arg) ;
+        newSetOwned->add(*arg) ;
       }
     }
-  } else {
-    newSet = (RooAbsCollection*) &newSetOrig ;
+    newSet = newSetOwned.get();
   }
 
   // Replace current servers with new servers with the same name from the given list
-  bool ret(false) ;
-
-  //Copy original server list to not confuse the iterator while deleting
-  std::vector<RooAbsArg*> origServerList, origServerValue, origServerShape;
-  auto origSize = _serverList.size();
-  origServerList.reserve(origSize);
-  origServerValue.reserve(origSize);
-
-  for (const auto oldServer : _serverList) {
-    origServerList.push_back(oldServer) ;
-
-    // Retrieve server side link state information
-    if (oldServer->_clientListValue.containsByNamePtr(this)) {
-      origServerValue.push_back(oldServer) ;
-    }
-    if (oldServer->_clientListShape.containsByNamePtr(this)) {
-      origServerShape.push_back(oldServer) ;
-    }
-  }
-
-  // Delete all previously registered servers
-  for (auto oldServer : origServerList) {
+  for (auto oldServer : _serverList) {
 
     RooAbsArg * newServer= oldServer->findNewServer(*newSet, nameChange);
 
@@ -1071,21 +1072,29 @@ bool RooAbsArg::redirectServers(const RooAbsCollection& newSetOrig, bool mustRep
       continue ;
     }
 
-    auto findByNamePtr = [&oldServer](const RooAbsArg * item) {
-      return oldServer->namePtr() == item->namePtr();
-    };
-    bool propValue = std::any_of(origServerValue.begin(), origServerValue.end(), findByNamePtr);
-    bool propShape = std::any_of(origServerShape.begin(), origServerShape.end(), findByNamePtr);
-
     if (newServer != this) {
-      replaceServer(*oldServer,*newServer,propValue,propShape) ;
+      _serverList.Replace(oldServer, newServer);
+
+      const int clientListRefCount = oldServer->_clientList.Remove(this, true);
+      const int clientListValueRefCount = oldServer->_clientListValue.Remove(this, true);
+      const int clientListShapeRefCount = oldServer->_clientListShape.Remove(this, true);
+
+      newServer->_clientList.Add(this, clientListRefCount);
+      newServer->_clientListValue.Add(this, clientListValueRefCount);
+      newServer->_clientListShape.Add(this, clientListShapeRefCount);
+
+      if (clientListValueRefCount > 0 && newServer->operMode() == ADirty && operMode() != ADirty) {
+        setOperMode(ADirty);
+      }
     }
   }
 
 
   setValueDirty() ;
   setShapeDirty() ;
 
+  bool ret(false) ;
+
   // Process the proxies
   for (int i=0 ; i<numProxies() ; i++) {
     RooAbsProxy* p = getProxy(i) ;
@@ -1108,10 +1117,6 @@ bool RooAbsArg::redirectServers(const RooAbsCollection& newSetOrig, bool mustRep
   }
   ret |= redirectServersHook(*newSet,mustReplaceAll,nameChange,isRecursionStep) ;
 
-  if (nameChange) {
-    delete newSet ;
-  }
-
   return ret ;
 }
 

roofit/roofitcore/src/RooGenProdProj.cxx

@@ -110,11 +110,6 @@ RooGenProdProj::RooGenProdProj(const RooGenProdProj& other, const char* name) :
   _compSetD("compSetD","Set of integral components owned by denominator",this),
   _intList("intList","List of integrals",this)
 {
-  // Explicitly remove all server links at this point
-  for(RooAbsArg * server : servers()) {
-    removeServer(*server,true) ;
-  }
-
   // Copy constructor
   _compSetOwnedN = new RooArgSet;
   other._compSetN.snapshot(*_compSetOwnedN);