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
Modernize analyzers in CondFormats/RPCObjects/test #36148
Modernize analyzers in CondFormats/RPCObjects/test #36148
Conversation
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-36148/26688
|
A new Pull Request was created by @mrodozov (Mircho Rodozov) for master. It involves the following packages:
@malbouis, @yuanchao, @cmsbuild, @ggovi, @francescobrivio, @tvami can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
edm::ESHandle<RPCEMap> readoutMapping = iSetup.getHandle(readoutMappingToken_); | ||
map = readoutMapping.product()->convert(); |
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 Handle is not used for anything so this could be make concise like
edm::ESHandle<RPCEMap> readoutMapping = iSetup.getHandle(readoutMappingToken_); | |
map = readoutMapping.product()->convert(); | |
const auto map = &iSetup.getData(readoutMappingToken_)->convert(); |
although I'm not really sure what the convert() is doing
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.
I removed the redundant vars but I'm not sure how to switch to getData instead of getHandle
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.
I gave an explicit suggestion here
#36148 (comment)
HTH
} else { | ||
edm::ESHandle<RPCReadOutMapping> map0; | ||
iSetup.get<RPCReadOutMappingRcd>().get(map0); | ||
edm::ESHandle<RPCReadOutMapping> map0 = iSetup.getHandle(mapZeroToken_); | ||
map = map0.product(); |
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.
This def can be
map = map0.product(); | |
const auto map = &iSetup.getData(mapZeroToken_); |
9ae8047
to
c94b375
Compare
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-36148/26690
|
edm::ESHandle<RPCReadOutMapping> map0; | ||
iSetup.get<RPCReadOutMappingRcd>().get(map0); | ||
map = map0.product(); | ||
map = iSetup.getHandle(mapZeroToken_).product(); |
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.
Hi @mrodozov this is a bit different from what I suggested. What suggested had the getData()
function.
getData()
is like the getHandle
and the .product()
together
I also suggested to make it const
(and to use auto
)
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.
map = iSetup.getHandle(mapZeroToken_).product(); | |
const auto map = &iSetup.getData(mapZeroToken_); |
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 map is defined outside the condition already
const RPCEMap* eMap = readoutMapping.product(); | ||
// RPCReadOutMapping* map = eMap->convert(); | ||
map = eMap->convert(); | ||
map = iSetup.getHandle(readoutMappingToken_).product()->convert(); |
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.
same thing here with the getData
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-36148/26691
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-36148/26699
|
@cmsbuild , please test |
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-e9f774/20588/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. @perrotta, @dpiparo, @qliphy (and backports should be raised in the release meeting by the corresponding L2) |
+1 |
PR description:
This moves analyzers in tests to esConsumes and uses one/EDAnalyzer as suggested in the
CMSSW_12_2_CMSDEPRECATED_X_2021 IB
Removes a dummy destructor
PR validation:
Tests ran, compiles without warnings
if this PR is a backport please specify the original PR and why you need to backport that PR: