Fix multiple comp warnings for unused variables #20841
Conversation
|
The code-checks are being triggered in jenkins. |
|
+code-checks |
|
A new Pull Request was created by @mrodozov for master. It involves the following packages: Calibration/Tools @perrotta, @ghellwig, @prebello, @vazzolini, @kmaeshima, @arunhep, @cerminar, @fabozzi, @cmsbuild, @GurpreetSinghChahal, @franzoni, @slava77, @ggovi, @lpernie, @vanbesien, @dmitrijus can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
|
please test |
|
The tests are being triggered in jenkins. |
| @@ -106,7 +106,6 @@ class FilterPFCandByParticleId { | |||
| id_(particleId){}; | |||
| template<typename PFCandCompatiblePtrType> | |||
| bool operator()(const PFCandCompatiblePtrType& ptr) const { | |||
| const PFCandidatePtr& pfptr(ptr); | |||
There was a problem hiding this comment.
@roger-wolf (just checking): wasn't this meant to check the particleId() of a PFCandidate (i.e. the particle type) even in case of different particleId() methods implemented for a generic PFCandCompatiblePtrType (that could even be pdgId instead of particle type)?
If so, the fix shold be returning here
return pfptr->particleId() == id_;
Otherwise, removing this line is perfectly fine.
|
+1 The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic: |
|
Comparison job queued. |
|
Comparison is ready Comparison Summary:
|
|
merge |
|
This got merged before waiting for an answer to my question in #20841 (review) Quite likely, it is perfectly fine doing so. However, before we forget it, could please @roger-wolf or @mbluj check whether that unused variable in RecoTauCrossCleaning.h was really added by mistake (perhaps copy-paste from some similar code), or at the contrary it was originally intended to force using the particleId() method of PFCandidate instead of the possibly different particleId() method of the generic PFCandCompatiblePtrType (thus witnessing a possible bug in the implementation that could be fixed independently, if so)? |
| @@ -397,7 +397,6 @@ void HLTJetMETValidation::getHLTResults(const edm::TriggerResults& hltresults, | |||
| HLTinit_=true; | |||
|
|
|||
| for (int itrig = 0; itrig != ntrigs; ++itrig){ | |||
There was a problem hiding this comment.
@mrodozov , we should delete the for loop too.
|
This got merged before waiting for an answer to my question in #20841
(review)
<#20841 (review)>
Quite likely, it is perfectly fine doing so.
However, before we forget it, could please @roger-wolf
<https://github.com/roger-wolf> or @mbluj <https://github.com/mbluj>
check whether that unused variable in RecoTauCrossCleaning.h was really
added by mistake (perhaps copy-paste from some similar code), or at the
contrary it was originally intended to force using the particleId() method
of PFCandidate instead of the possibly different particleId() method of the
generic PFCandCompatiblePtrType (thus witnessing a possible bug in the
implementation that could be fixed independently, if so)?
The () operator of of the FilterPFCandByParticleId template class is indeed
intended to return truth if the type of examined PFCandidate (
ptr->particleId()) is equal to the type used to initialize an object of the
class (id_(particleId)). The class is used massively in the tau reco code
to filter input PFCandidates and works fine. I do know what the
intermediate pfptr is/was needed for, but obviously it is not used. It
could be that you are right that the intention was to protect against
(possible) different implementation of the particleId() method in the
generic PFCandCompatiblePtrType class. So even if the
FilterPFCandByParticleId class has been working fine for a long time it is
probably wise to use proposed fix rather than remove the line.
|
|
Thank you @mbluj for having checked! |
|
Thank you @mbluj <https://github.com/mbluj> for having checked!
I let you guys decide whether and when to make a PR with the proposed fix,
if you think so.
OK.
Anyhow, this PR do not change the behaviour of your filters, and I think
it can remain merged as it is now.
It makes sense to me.
And lust, but not least: thanks for notifying us.
|
Removes all compilation warnings due to unused vars listed here:
https://cmssdt.cern.ch/SDT/cgi-bin/showBuildLogs.py/slc6_amd64_gcc630/www/sun/9.4-sun-11/CMSSW_9_4_X_2017-10-08-1100