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

Fix deprecated-copy warnings in L1CaloTrigger #43771

Merged
merged 1 commit into from
Jan 23, 2024
Merged

Fix deprecated-copy warnings in L1CaloTrigger #43771

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
48 changes: 6 additions & 42 deletions L1Trigger/L1CaloTrigger/interface/Phase2L1CaloEGammaUtils.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -444,12 +444,6 @@ namespace p2eg {
fb = 0;
};

// copy constructor
towerHCAL(const towerHCAL& other) {
et = other.et;
fb = other.fb;
};

// set members
inline void zeroOut() {
et = 0;
Expand Down Expand Up @@ -486,6 +480,12 @@ namespace p2eg {
};
};

// overload operator= to use copy constructor
towers3x4 operator=(const towers3x4& other) {
const towers3x4& newTowers3x4(other);
return newTowers3x4;
};
Copy link
Contributor

Choose a reason for hiding this comment

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

This copy assignment operator does not follow the usual semantics. It constructs a new towers3x4 object from the argument, and returns that new object (or a copy of the new object if for any reason the compiler won't elide the copy), and leaves this object untouched. The behavior should be that the contents of the argument are copied to this, and *this is returned by reference.

How about leaving both copy assignment and copy constructor to be generated by the compiler?

(on a further note, independently of this PR, the names towers3x4 and towerHCAL are against our naming convention, class names should start with capital letter; and the towersHCAL data member name should have a trailing underscore, i.e. be towersHCAL_).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How about leaving both copy assignment and copy constructor to be generated by the compiler?

I'm not sure default copy-constructor/copy-assignment operator will correctly copy towersHCAL 2D array.

This copy assignment operator does not follow the usual semantics. It constructs a new towers3x4 object from the argument, and returns that new object (or a copy of the new object if for any reason the compiler won't elide the copy), and leaves this object untouched. The behavior should be that the contents of the argument are copied to this, and *this is returned by reference.

Should we also change region3x4::operator=?

Copy link
Contributor

Choose a reason for hiding this comment

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

How about leaving both copy assignment and copy constructor to be generated by the compiler?

I'm not sure default copy-constructor/copy-assignment operator will correctly copy towersHCAL 2D array.

Ah right, C-arrays don't have copy operators 🤦 . Maybe the towersHCAL could be changed to be nested std::array (naively looks straightforward), after which the copy (and move) operations would be defined automatically?

This copy assignment operator does not follow the usual semantics. It constructs a new towers3x4 object from the argument, and returns that new object (or a copy of the new object if for any reason the compiler won't elide the copy), and leaves this object untouched. The behavior should be that the contents of the argument are copied to this, and *this is returned by reference.

Should we also change region3x4::operator=?

Yes, that should be fixed as well.


// set members
inline void zeroOut() {
for (int i = 0; i < TOWER_IN_ETA; i++) {
Expand Down Expand Up @@ -606,13 +606,6 @@ namespace p2eg {
phiMax = 0;
etaMax = 0;
}

crystalMax& operator=(const crystalMax& rhs) {
energy = rhs.energy;
phiMax = rhs.phiMax;
etaMax = rhs.etaMax;
return *this;
}
};

class ecaltp_t {
Expand Down Expand Up @@ -689,10 +682,6 @@ namespace p2eg {
ap_uint<16> data;

tower_t() { data = 0; }
tower_t& operator=(const tower_t& rhs) {
data = rhs.data;
return *this;
}

tower_t(ap_uint<12> et, ap_uint<4> hoe) { data = (et) | (((ap_uint<16>)hoe) << 12); }

Expand Down Expand Up @@ -780,17 +769,6 @@ namespace p2eg {
etaMax = 0;
brems = 0;
}

clusterInfo& operator=(const clusterInfo& rhs) {
seedEnergy = rhs.seedEnergy;
energy = rhs.energy;
et5x5 = rhs.et5x5;
et2x5 = rhs.et2x5;
phiMax = rhs.phiMax;
etaMax = rhs.etaMax;
brems = rhs.brems;
return *this;
}
};

//--------------------------------------------------------//
Expand Down Expand Up @@ -851,20 +829,6 @@ namespace p2eg {
is_looseTkiso = cluster_is_looseTkiso;
}

Cluster& operator=(const Cluster& rhs) {
data = rhs.data;
regionIdx = rhs.regionIdx;
calib = rhs.calib;
brems = rhs.brems;
et5x5 = rhs.et5x5;
et2x5 = rhs.et2x5;
is_ss = rhs.is_ss;
is_looseTkss = rhs.is_looseTkss;
is_iso = rhs.is_iso;
is_looseTkiso = rhs.is_looseTkiso;
return *this;
}

void setRegionIdx(int regIdx) { regionIdx = regIdx; } // Newly added

ap_uint<12> clusterEnergy() const { return (data & 0xFFF); }
Expand Down