Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 6 commits into from
Jan 4, 2023
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
5e90eab
Fix use-after-move in L1MuDTTrackFinder
makortel Dec 28, 2022
48a9800
Replace owning raw pointers with std::unique_ptr
makortel Dec 28, 2022
2f86220
const function needs to return pointer to const
makortel Dec 28, 2022
c52652f
Remove unnecessary virtual function specifiers
makortel Dec 28, 2022
d54a841
Make config() non-static member function
makortel Dec 28, 2022
3f7b803
Use range for loop
makortel Dec 28, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 10 additions & 17 deletions L1Trigger/DTTrackFinder/interface/L1MuDTTrackFinder.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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();
Expand All @@ -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) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This function appeared to be unused so I removed it.

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
2 changes: 1 addition & 1 deletion L1Trigger/DTTrackFinder/src/L1MuDTEtaProcessor.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand Down
2 changes: 1 addition & 1 deletion L1Trigger/DTTrackFinder/src/L1MuDTEtaProcessor.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
17 changes: 5 additions & 12 deletions L1Trigger/DTTrackFinder/src/L1MuDTSecProcMap.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 --
Expand All @@ -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);
}
9 changes: 5 additions & 4 deletions L1Trigger/DTTrackFinder/src/L1MuDTSecProcMap.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
//---------------

#include <map>
#include <memory>

//----------------------
// Base Class Headers --
Expand All @@ -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(); }
Expand Down
133 changes: 45 additions & 88 deletions L1Trigger/DTTrackFinder/src/L1MuDTTrackFinder.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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);
Expand All @@ -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 --
Expand All @@ -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);
}

//
Expand Down Expand Up @@ -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();
Comment on lines 162 to -191
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This pattern of

if (debug)
  // access ptr
if (ptr)
  // access ptr
if (debug && ptr)
  // access ptr

seems to repeat in this file many times. Given how the vectors and the map are filled, there should not be any nullptr entries, and therefore the if (ptr) check seems redundant. Also the nullness is not checked in the first if, so the code would (likely) crash anyway if, for any reason, the ptr would be null. Could this pattern be replaced with just

if (debug)
  // access ptr
// access ptr
if (debug)
  // access ptr

?

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(),
Expand All @@ -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
Expand All @@ -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()));
}
}
}
Expand All @@ -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)
Expand Down Expand Up @@ -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;