[ALCA] Fix copy-constructor warnings reported in DEVEL IBs #43423
Conversation
|
-code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-43423/37938
Code check has found code style and quality issues which could be resolved by applying following patch(s)
|
cmsbuild
added
code-checks-rejected
code-checks-pending
and removed
code-checks-pending
code-checks-rejected
labels
| @@ -29,6 +29,9 @@ class SiStripHashedDetId { | |||
| /** Copy constructor. */ | |||
| SiStripHashedDetId(const SiStripHashedDetId &); | |||
|
|
|||
| /** Assignment operator. */ | |||
| const SiStripHashedDetId &operator=(const SiStripHashedDetId &) { return *this; } | |||
There was a problem hiding this comment.
@aandvalenzuela are you sure this is correct?
There was a problem hiding this comment.
No, I am not. This is what the code format reported (see https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-43423/37938/code-format.patch). I was actually taking this issue offline... any idea why the code formatting is reporting on code that has not been touched?
There was a problem hiding this comment.
I mean - are you sure you have implemented operator= correctly?
There was a problem hiding this comment.
I think that is the default implementation, but I am not familiar with it. Do you have any suggestions?
There was a problem hiding this comment.
Does something like this look better?
TSinglePedEntry& operator=(const TSinglePedEntry& orig) {
m_pedestalSqSum = orig.m_pedestalSqSum;
m_pedestalSum = orig.m_pedestalSum;
m_entries = orig.m_entries;
return *this;
}
There was a problem hiding this comment.
Mmm...
TSinglePedEntry& operator=(const TSinglePedEntry& orig) = default;
CalibCalorimetry/EcalPedestalOffsets/interface/TSinglePedEntry.h
Outdated
Show resolved
Hide resolved
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-43423/37940
|
|
A new Pull Request was created by @aandvalenzuela (Andrea Valenzuela) for master. It involves the following packages:
@consuegs, @cmsbuild, @saumyaphor4252, @perrotta can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-43423/37941
|
|
Pull request #43423 was updated. @perrotta, @cmsbuild, @consuegs, @saumyaphor4252 can you please check and sign again. |
Apply code format Apply code format Use default implementation
aandvalenzuela
force-pushed
the
alca-from-CMSSW_14_0_DEVEL_X_2023-11-27-2300
branch
from
f4b0135 to
70f2763
Compare
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-43423/37943
|
|
Pull request #43423 was updated. @perrotta, @consuegs, @saumyaphor4252, @cmsbuild can you please check and sign again. |
|
please test |
|
please test for CMSSW_14_0_DEVEL_X |
|
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-48da4d/36134/summary.html Comparison SummarySummary:
|
|
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-48da4d/36136/summary.html Comparison SummarySummary:
|
|
+alca
|
cmsbuild
added
alca-approved
fully-signed
and removed
alca-pending
pending-signatures
labels
|
This pull request is fully signed and it will be integrated in one of the next master IBs (tests are also fine). This pull request will now be reviewed by the release team before it's merged. @rappoccio, @sextonkennedy, @antoniovilela (and backports should be raised in the release meeting by the corresponding L2) |
|
+1 |
| @@ -29,6 +29,9 @@ class SiStripHashedDetId { | |||
| /** Copy constructor. */ | |||
| SiStripHashedDetId(const SiStripHashedDetId &); | |||
|
|
|||
| /** Assignment operator. */ | |||
| SiStripHashedDetId &operator=(const SiStripHashedDetId &) = default; | |||
There was a problem hiding this comment.
@aandvalenzuela @perrotta
unfortunately this leads to wrong intializations of the assigned object. I fixed this at 3eda318 (incidentally also the copy constructor right above was buggy).
Hello,
This PR follows up on the discussion started at #43408.
I am defining the missing assignment operators to fulfill the rule of three (see #43408 (comment)) to avoid missing initializations (see #43408 (comment))
Thanks!