Merge pull request #40411 from makortel/fixMoveL1MuDTTrackFinder

Fix use-after-move in L1MuDTTrackFinder, and some additional modernization

L1Trigger/DTTrackFinder/interface/L1MuDTTrackFinder.h

@@ -64,7 +64,7 @@ class L1MuDTTrackFinder {
   L1MuDTTrackFinder(const edm::ParameterSet& ps, edm::ConsumesCollector&& iC);
 
   /// destructor
-  virtual ~L1MuDTTrackFinder();
+  ~L1MuDTTrackFinder();
 
   /// build the structure of the barrel MTTF
   void setup(edm::ConsumesCollector&& iC);
@@ -79,13 +79,13 @@ class L1MuDTTrackFinder {
   const L1MuDTSectorProcessor* sp(const L1MuDTSecProcId&) const;
 
   /// get a pointer to an Eta Processor, index [0-11]
-  inline const L1MuDTEtaProcessor* ep(int id) const { return m_epvec[id]; }
+  inline const L1MuDTEtaProcessor* ep(int id) const { return m_epvec[id].get(); }
 
   /// get a pointer to a Wedge Sorter, index [0-11]
-  inline const L1MuDTWedgeSorter* ws(int id) const { return m_wsvec[id]; }
+  inline const L1MuDTWedgeSorter* ws(int id) const { return m_wsvec[id].get(); }
 
   /// get a pointer to the DT Muon Sorter
-  inline const L1MuDTMuonSorter* ms() const { return m_ms; }
+  inline const L1MuDTMuonSorter* ms() const { return m_ms.get(); }
 
   /// get number of muon candidates found by the barrel MTTF
   int numberOfTracks();
@@ -100,29 +100,22 @@ class L1MuDTTrackFinder {
   int numberOfTracks(int bx);
 
   /// return configuration
-  static L1MuDTTFConfig* config() { return m_config.get(); }
+  const L1MuDTTFConfig* config() const { return m_config.get(); }
 
   std::vector<L1MuDTTrackCand>& getcache0() { return _cache0; }
 
   std::vector<L1MuRegionalCand>& getcache() { return _cache; }
 
-private:
-  /// run Track Finder and store candidates in cache
-  virtual void reconstruct(const edm::Event& e, const edm::EventSetup& c) {
-    reset();
-    run(e, c);
-  }
-
 private:
   std::vector<L1MuDTTrackCand> _cache0;
   std::vector<L1MuRegionalCand> _cache;
-  L1MuDTSecProcMap* m_spmap;                 ///< Sector Processors
-  std::vector<L1MuDTEtaProcessor*> m_epvec;  ///< Eta Processors
-  std::vector<L1MuDTWedgeSorter*> m_wsvec;   ///< Wedge Sorters
-  L1MuDTMuonSorter* m_ms;                    ///< DT Muon Sorter
+  std::unique_ptr<L1MuDTSecProcMap> m_spmap;                 ///< Sector Processors
+  std::vector<std::unique_ptr<L1MuDTEtaProcessor>> m_epvec;  ///< Eta Processors
+  std::vector<std::unique_ptr<L1MuDTWedgeSorter>> m_wsvec;   ///< Wedge Sorters
+  std::unique_ptr<L1MuDTMuonSorter> m_ms;                    ///< DT Muon Sorter
   edm::EDGetTokenT<L1MuDTChambPhContainer> m_DTDigiToken;
 
-  static std::shared_ptr<L1MuDTTFConfig> m_config;  ///< Track Finder configuration
+  std::unique_ptr<L1MuDTTFConfig> m_config;  ///< Track Finder configuration
 };
 
 #endif

L1Trigger/DTTrackFinder/src/L1MuDTEtaProcessor.cc

@@ -60,7 +60,7 @@ using namespace std;
 // Constructors --
 //----------------
 
-L1MuDTEtaProcessor::L1MuDTEtaProcessor(const L1MuDTTrackFinder& tf, int id, edm::ConsumesCollector&& iC)
+L1MuDTEtaProcessor::L1MuDTEtaProcessor(const L1MuDTTrackFinder& tf, int id, edm::ConsumesCollector iC)
     : m_tf(tf),
       m_epid(id),
       m_foundPattern(0),

L1Trigger/DTTrackFinder/src/L1MuDTEtaProcessor.h

@@ -58,7 +58,7 @@ class L1MuDTQualPatternLutRcd;
 class L1MuDTEtaProcessor {
 public:
   /// constructor
-  L1MuDTEtaProcessor(const L1MuDTTrackFinder&, int id, edm::ConsumesCollector&& iC);
+  L1MuDTEtaProcessor(const L1MuDTTrackFinder&, int id, edm::ConsumesCollector iC);
 
   /// destructor
   virtual ~L1MuDTEtaProcessor();

L1Trigger/DTTrackFinder/src/L1MuDTSecProcMap.cc

@@ -46,14 +46,7 @@ L1MuDTSecProcMap::L1MuDTSecProcMap() : m_map() {}
 // Destructor --
 //--------------
 
-L1MuDTSecProcMap::~L1MuDTSecProcMap() {
-  SPmap_iter iter = m_map.begin();
-  while (iter != m_map.end()) {
-    delete (*iter).second;
-    iter++;
-  }
-  m_map.clear();
-}
+L1MuDTSecProcMap::~L1MuDTSecProcMap() = default;
 
 //--------------
 // Operations --
@@ -62,22 +55,22 @@ L1MuDTSecProcMap::~L1MuDTSecProcMap() {
 //
 // return Sector Processor
 //
-L1MuDTSectorProcessor* L1MuDTSecProcMap::sp(const L1MuDTSecProcId& id) const {
+const L1MuDTSectorProcessor* L1MuDTSecProcMap::sp(const L1MuDTSecProcId& id) const {
   SPmap::const_iterator it = m_map.find(id);
   if (it == m_map.end()) {
     //    cerr << "Error: Sector Processor not in the map" << endl;
     return nullptr;
   }
-  return (*it).second;
+  return (*it).second.get();
 }
 
 //
 // insert Sector Processor into container
 //
-void L1MuDTSecProcMap::insert(const L1MuDTSecProcId& id, L1MuDTSectorProcessor* sp) {
+void L1MuDTSecProcMap::insert(const L1MuDTSecProcId& id, std::unique_ptr<L1MuDTSectorProcessor> sp) {
   //SPmap::const_iterator it = m_map.find(id);
   //  if ( it != m_map.end() )
   //    cerr << "Error: More than one Sector Processor with same identifier"
   //         << endl;
-  m_map[id] = sp;
+  m_map[id] = std::move(sp);
 }

L1Trigger/DTTrackFinder/src/L1MuDTSecProcMap.h

@@ -19,6 +19,7 @@
 //---------------
 
 #include <map>
+#include <memory>
 
 //----------------------
 // Base Class Headers --
@@ -37,20 +38,20 @@ class L1MuDTSectorProcessor;
 
 class L1MuDTSecProcMap {
 public:
-  typedef std::map<L1MuDTSecProcId, L1MuDTSectorProcessor*, std::less<L1MuDTSecProcId> > SPmap;
+  typedef std::map<L1MuDTSecProcId, std::unique_ptr<L1MuDTSectorProcessor>> SPmap;
   typedef SPmap::iterator SPmap_iter;
 
   /// constructor
   L1MuDTSecProcMap();
 
   /// destructor
-  virtual ~L1MuDTSecProcMap();
+  ~L1MuDTSecProcMap();
 
   /// return pointer to Sector Processor
-  L1MuDTSectorProcessor* sp(const L1MuDTSecProcId&) const;
+  const L1MuDTSectorProcessor* sp(const L1MuDTSecProcId&) const;
 
   /// insert a Sector Processor into the container
-  void insert(const L1MuDTSecProcId&, L1MuDTSectorProcessor* sp);
+  void insert(const L1MuDTSecProcId&, std::unique_ptr<L1MuDTSectorProcessor> sp);
 
   /// return number of entries present in the container
   inline int size() const { return m_map.size(); }

L1Trigger/DTTrackFinder/src/L1MuDTTrackFinder.cc

@@ -53,11 +53,7 @@ using namespace std;
 
 L1MuDTTrackFinder::L1MuDTTrackFinder(const edm::ParameterSet& ps, edm::ConsumesCollector&& iC) {
   // set configuration parameters
-  if (not m_config) {
-    auto temp = std::make_shared<L1MuDTTFConfig>(ps);
-    std::shared_ptr<L1MuDTTFConfig> empty;
-    std::atomic_compare_exchange_strong(&m_config, &empty, temp);
-  }
+  m_config = std::make_unique<L1MuDTTFConfig>(ps);
 
   if (m_config->Debug(1))
     cout << endl;
@@ -66,10 +62,9 @@ L1MuDTTrackFinder::L1MuDTTrackFinder(const edm::ParameterSet& ps, edm::ConsumesC
   if (m_config->Debug(1))
     cout << endl;
 
-  m_spmap = new L1MuDTSecProcMap();
+  m_spmap = std::make_unique<L1MuDTSecProcMap>();
   m_epvec.reserve(12);
   m_wsvec.reserve(12);
-  m_ms = nullptr;
 
   _cache.reserve(4 * 17);
   _cache0.reserve(144 * 17);
@@ -81,25 +76,7 @@ L1MuDTTrackFinder::L1MuDTTrackFinder(const edm::ParameterSet& ps, edm::ConsumesC
 // Destructor --
 //--------------
 
-L1MuDTTrackFinder::~L1MuDTTrackFinder() {
-  delete m_spmap;
-
-  vector<L1MuDTEtaProcessor*>::iterator it_ep = m_epvec.begin();
-  while (it_ep != m_epvec.end()) {
-    delete (*it_ep);
-    it_ep++;
-  }
-  m_epvec.clear();
-
-  vector<L1MuDTWedgeSorter*>::iterator it_ws = m_wsvec.begin();
-  while (it_ws != m_wsvec.end()) {
-    delete (*it_ws);
-    it_ws++;
-  }
-  m_wsvec.clear();
-
-  delete m_ms;
-}
+L1MuDTTrackFinder::~L1MuDTTrackFinder() = default;
 
 //--------------
 // Operations --
@@ -124,29 +101,29 @@ void L1MuDTTrackFinder::setup(edm::ConsumesCollector&& iC) {
       continue;
     for (int sc = 0; sc < 12; sc++) {
       L1MuDTSecProcId tmpspid(wh, sc);
-      L1MuDTSectorProcessor* sp = new L1MuDTSectorProcessor(*this, tmpspid, std::move(iC));
+      auto sp = std::make_unique<L1MuDTSectorProcessor>(*this, tmpspid, iC);
       if (m_config->Debug(2))
         cout << "creating " << tmpspid << endl;
-      m_spmap->insert(tmpspid, sp);
+      m_spmap->insert(tmpspid, std::move(sp));
     }
   }
 
   // create new eta processors and wedge sorters
   for (int sc = 0; sc < 12; sc++) {
-    L1MuDTEtaProcessor* ep = new L1MuDTEtaProcessor(*this, sc, std::move(iC));
+    auto ep = std::make_unique<L1MuDTEtaProcessor>(*this, sc, iC);
     if (m_config->Debug(2))
       cout << "creating Eta Processor " << sc << endl;
-    m_epvec.push_back(ep);
-    L1MuDTWedgeSorter* ws = new L1MuDTWedgeSorter(*this, sc);
+    m_epvec.push_back(std::move(ep));
+    auto ws = std::make_unique<L1MuDTWedgeSorter>(*this, sc);
     if (m_config->Debug(2))
       cout << "creating Wedge Sorter " << sc << endl;
-    m_wsvec.push_back(ws);
+    m_wsvec.push_back(std::move(ws));
   }
 
   // create new muon sorter
   if (m_config->Debug(2))
     cout << "creating DT Muon Sorter " << endl;
-  m_ms = new L1MuDTMuonSorter(*this);
+  m_ms = std::make_unique<L1MuDTMuonSorter>(*this);
 }
 
 //
@@ -181,36 +158,31 @@ void L1MuDTTrackFinder::run(const edm::Event& e, const edm::EventSetup& c) {
     reset();
 
     // run sector processors
-    L1MuDTSecProcMap::SPmap_iter it_sp = m_spmap->begin();
-    while (it_sp != m_spmap->end()) {
+    for (auto& sp : *m_spmap) {
       if (m_config->Debug(2))
-        cout << "running " << (*it_sp).second->id() << endl;
-      if ((*it_sp).second)
-        (*it_sp).second->run(bx, e, c);
-      if (m_config->Debug(2) && (*it_sp).second)
-        (*it_sp).second->print();
-      it_sp++;
+        cout << "running " << sp.second->id() << endl;
+      if (sp.second)
+        sp.second->run(bx, e, c);
+      if (m_config->Debug(2) && sp.second)
+        sp.second->print();
     }
 
     // run eta processors
-    vector<L1MuDTEtaProcessor*>::iterator it_ep = m_epvec.begin();
-    while (it_ep != m_epvec.end()) {
+    for (auto& ep : m_epvec) {
       if (m_config->Debug(2))
-        cout << "running Eta Processor " << (*it_ep)->id() << endl;
-      if (*it_ep)
-        (*it_ep)->run(bx, e, c);
-      if (m_config->Debug(2) && *it_ep)
-        (*it_ep)->print();
-      it_ep++;
+        cout << "running Eta Processor " << ep->id() << endl;
+      if (ep)
+        ep->run(bx, e, c);
+      if (m_config->Debug(2) && ep)
+        ep->print();
     }
 
     // read sector processors
-    it_sp = m_spmap->begin();
-    while (it_sp != m_spmap->end()) {
+    for (auto& sp : *m_spmap) {
       if (m_config->Debug(2))
-        cout << "reading " << (*it_sp).second->id() << endl;
+        cout << "reading " << sp.second->id() << endl;
       for (int number = 0; number < 2; number++) {
-        const L1MuDTTrack* cand = (*it_sp).second->tracK(number);
+        const L1MuDTTrack* cand = sp.second->tracK(number);
         if (cand && !cand->empty())
           _cache0.push_back(L1MuDTTrackCand(cand->getDataWord(),
                                             cand->bx(),
@@ -223,19 +195,16 @@ void L1MuDTTrackFinder::run(const edm::Event& e, const edm::EventSetup& c) {
                                             cand->address(4),
                                             cand->tc()));
       }
-      it_sp++;
     }
 
     // run wedge sorters
-    vector<L1MuDTWedgeSorter*>::iterator it_ws = m_wsvec.begin();
-    while (it_ws != m_wsvec.end()) {
+    for (auto& ws : m_wsvec) {
       if (m_config->Debug(2))
-        cout << "running Wedge Sorter " << (*it_ws)->id() << endl;
-      if (*it_ws)
-        (*it_ws)->run();
-      if (m_config->Debug(2) && *it_ws)
-        (*it_ws)->print();
-      it_ws++;
+        cout << "running Wedge Sorter " << ws->id() << endl;
+      if (ws)
+        ws->run();
+      if (m_config->Debug(2) && ws)
+        ws->print();
     }
 
     // run muon sorter
@@ -248,11 +217,9 @@ void L1MuDTTrackFinder::run(const edm::Event& e, const edm::EventSetup& c) {
 
     // store found track candidates in container (cache)
     if (m_ms->numberOfTracks() > 0) {
-      const vector<const L1MuDTTrack*>& mttf_cont = m_ms->tracks();
-      vector<const L1MuDTTrack*>::const_iterator iter;
-      for (iter = mttf_cont.begin(); iter != mttf_cont.end(); iter++) {
-        if (*iter)
-          _cache.push_back(L1MuRegionalCand((*iter)->getDataWord(), (*iter)->bx()));
+      for (auto const& mttf : m_ms->tracks()) {
+        if (mttf)
+          _cache.push_back(L1MuRegionalCand(mttf->getDataWord(), mttf->bx()));
       }
     }
   }
@@ -262,25 +229,19 @@ void L1MuDTTrackFinder::run(const edm::Event& e, const edm::EventSetup& c) {
 // reset MTTF
 //
 void L1MuDTTrackFinder::reset() {
-  L1MuDTSecProcMap::SPmap_iter it_sp = m_spmap->begin();
-  while (it_sp != m_spmap->end()) {
-    if ((*it_sp).second)
-      (*it_sp).second->reset();
-    it_sp++;
+  for (auto& sp : *m_spmap) {
+    if (sp.second)
+      sp.second->reset();
   }
 
-  vector<L1MuDTEtaProcessor*>::iterator it_ep = m_epvec.begin();
-  while (it_ep != m_epvec.end()) {
-    if (*it_ep)
-      (*it_ep)->reset();
-    it_ep++;
+  for (auto& ep : m_epvec) {
+    if (ep)
+      ep->reset();
   }
 
-  vector<L1MuDTWedgeSorter*>::iterator it_ws = m_wsvec.begin();
-  while (it_ws != m_wsvec.end()) {
-    if (*it_ws)
-      (*it_ws)->reset();
-    it_ws++;
+  for (auto& ws : m_wsvec) {
+    if (ws)
+      ws->reset();
   }
 
   if (m_ms)
@@ -311,14 +272,10 @@ void L1MuDTTrackFinder::clear() {
 //
 int L1MuDTTrackFinder::numberOfTracks(int bx) {
   int number = 0;
-  for (TFtracks_const_iter it = _cache.begin(); it != _cache.end(); it++) {
-    if ((*it).bx() == bx)
+  for (auto const& elem : _cache) {
+    if (elem.bx() == bx)
       number++;
   }
 
   return number;
 }
-
-// static data members
-
-std::shared_ptr<L1MuDTTFConfig> L1MuDTTrackFinder::m_config;