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.
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
Implement support for Event::getByToken for a GenericHandle #32620
Conversation
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-32620/20675
|
A new Pull Request was created by @fwyzard (Andrea Bocci) for master. It involves the following packages: FWCore/Framework @makortel, @smuzaffar, @cmsbuild, @Dr15Jones can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
@cmsbuild, please test |
///Specialize the Event's getByToken method to work with a Handle<GenericObject> | ||
template <> | ||
bool Event::getByToken<GenericObject>(EDGetToken token, Handle<GenericObject>& result) const { | ||
result.clear(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As @fwyzard mentioned in mattermost, the implementations of the getByLabel()
specializations have diverged from the generic getByLabel()
, whereas the added getByToken()
specialization follows the structure of the generic getByToken()
. @Dr15Jones would you agree it would be good to update the getByLabel()
specializations (in a separate PR unless @fwyzard would be willing to do that here as well)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the differences are not actually functional (I can't really tell with a cursory look) than an update to make it consistent would make sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only possibly functional difference I see is that the generic getByLabel()
has this result.clear()
call, while the specializations above do not have it.
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-649f91/12036/summary.html Comparison SummarySummary:
|
+1 |
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. @silviodonato, @dpiparo, @qliphy (and backports should be raised in the release meeting by the corresponding L2) |
+1 |
PR description:
Implement support for
edm::Event::getByToken
for anedm::GenericHandle
.PR validation:
None.