Added an edm::ValidHandle type #25682
Conversation
The new handle type is guaranteed to be valid so no additional checks are done internally when it is being used.
We switched to always using C++11 a long time ago.
|
The code-checks are being triggered in jenkins. |
|
@guitargeek Here is the first change. |
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-25682/8035
|
|
A new Pull Request was created by @Dr15Jones (Chris Jones) for master. It involves the following packages: DataFormats/Common @cmsbuild, @smuzaffar, @Dr15Jones can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
|
please test |
|
The tests are being triggered in jenkins. |
| } | ||
| ValidHandle( const ValidHandle<T>& ) = default; | ||
| ValidHandle<T>& operator=(ValidHandle<T> const& rhs) = default; | ||
| ~ValidHandle() = default; |
There was a problem hiding this comment.
What about moves? (actually simplest way, as we just reminded ourselves, would be to remove the destructor and copy constructor/assignment definitions and leave all 5 to be generated by the compiler)
There was a problem hiding this comment.
I didn't really want moves. The moves are no different from what a copy should do and I explicitly do NOT want a move of a ValidHandle to cause the handle to become invalid :).
There was a problem hiding this comment.
What about explicitly defining the move operations deleted then (so that the intention is clear)?
There was a problem hiding this comment.
Ok, we discussed in length.
|
@smuzaffar why didn't the build test report a problem? |
|
please test |
|
The tests are being triggered in jenkins. |
cmsbuild
added
tests-pending
tests-started
and removed
tests-started
tests-pending
labels
|
Comparison job queued. |
|
+1 |
cmsbuild
added
core-approved
fully-signed
and removed
core-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. @davidlange6, @slava77, @smuzaffar, @fabiocos (and backports should be raised in the release meeting by the corresponding L2) |
|
Comparison is ready Comparison Summary:
|
|
@Dr15Jones , looks like bug in testing script. When unittests are timed out then it fails the whole job and does not report back. In this case unit tests were timed out after 7 hours [a]. I have updated the script to continue and report back the failure. [a] |
|
@Dr15Jones the changes are mostly in the L1T domain, I do not see a specific reason related to this PR for them, apart possible non reproducibility of that part of code, do you agree? |
|
My changed primarily added a new class not used anywhere in CMSSW. Therefore I'd say the changes from the test runs are irrelevant. |
|
+1 |
A
edm::ValidHandle<>is more efficient to use in different functions than aedm::Handle<>since the latter always checks to see if it is valid or not.