-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Changes non-const statics in PFCluster to avoid static analyzer warnings #1345
Changes non-const statics in PFCluster to avoid static analyzer warnings #1345
Conversation
The static analyzer flagged several non-const statics in this class as potential threaded problems. We removed instanceCounter_ since it was never used and moved the dummyVtx function static used as a return value to be a const class static. The statics associated with setDepthCorParameters and getDepthCorParameters were left as is but they were reported to the appropriate L2s.
A new Pull Request was created by @Dr15Jones (Chris Jones) for CMSSW_7_0_X. Changes non-const statics in PFCluster to avoid static analyzer warnings It involves the following packages: DataFormats/ParticleFlowReco @nclopezo, @cmsbuild, @thspeer, @slava77 can you please review it and eventually sign? Thanks. |
@@ -130,8 +128,7 @@ | |||
|
|||
/// dummy vertex access | |||
math::XYZPoint const & vertex() const { |
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 is light enough that it could've been returned by value.
Any reason to think otherwise?
In fact, considering it's inlined, it'll probably be optimized away to something trivial.
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 had thought it might be a virtual function override from the base class so the signature couldn't change. But that is not the case. If you want, I can make the change you suggested. Of course, one can wonder why those methods exist at all.
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 Chris,
Uhm, reading the notes above:
/// some classes to make this fit into a template footprint
/// for RecoPFClusterRefCandidate so we can make jets and MET
/// out of PFClusters.
So, this is trying to fit the signature of
DataFormats/Candidate/interface/LeafRefCandidateT.h
@rappoccio
Sal should point better to the right place.
Maybe a reference is not necessary.
--slava
On 11/6/13, 4:31 PM, Chris Jones wrote:
In DataFormats/ParticleFlowReco/interface/PFCluster.h:
@@ -130,8 +128,7 @@
/// dummy vertex access math::XYZPoint const & vertex() const {
I had thought it might be a virtual function override from the base
class so the signature couldn't change. But that is not the case. If you
want, I can make the change you suggested. Of course, one can wonder why
those methods exist at all.—
Reply to this email directly or view it on GitHub
https://github.com/cms-sw/cmssw/pull/1345/files#r7466580.
Vyacheslav (Slava) Krutelyov
TAMU: Physics Dept Texas A&M MS4242, College Station, TX 77843-4242
CERN: 42-R-027
AIM/Skype: siava16 googleTalk: slava77@gmail.com
(630) 291-5128 Cell (US) +41 76 275 7116 Cell (CERN)
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.
IIRC we do need the reference, defined here :
http://cmslxr.fnal.gov/lxr/source/DataFormats/Candidate/interface/LeafRefCandidateT.h#136
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.
Sal,
Where are the actual template methods?
Do they have point& in them or do they actually convert to values
when calling T.vertex
--slava
On 11/6/13, 5:38 PM, rappoccio wrote:
In DataFormats/ParticleFlowReco/interface/PFCluster.h:
@@ -130,8 +128,7 @@
/// dummy vertex access math::XYZPoint const & vertex() const {
IIRC we do need the reference, defined here :
http://cmslxr.fnal.gov/lxr/source/DataFormats/Candidate/interface/LeafRefCandidateT.h#136
—
Reply to this email directly or view it on GitHub
https://github.com/cms-sw/cmssw/pull/1345/files#r7469269.
Vyacheslav (Slava) Krutelyov
TAMU: Physics Dept Texas A&M MS4242, College Station, TX 77843-4242
CERN: 42-R-027
AIM/Skype: siava16 googleTalk: slava77@gmail.com
(630) 291-5128 Cell (US) +41 76 275 7116 Cell (CERN)
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, Slava,
Maybe I'm confused… line 136 of LeafRefCandidateT is the template method ("return ref_->vertex();", which calls T::vertex() ). Or is this not what you're asking?
Cheers,
Sal
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 Sal,
Thanks for pointing to the obvious, I confused myself and failed to read the method implementation.
OK then, no further comments on this PR.
Working @thspeer |
+1 |
This pull request is fully signed and it will be integrated in one of the next IBs unless changes or unless it breaks tests. @ktf can you please take care of it? |
…ticleFlowReco Multithreading fixes -- Changes non-const statics in PFCluster to avoid static analyzer warnings
The static analyzer flagged several non-const statics in this class as potential threaded problems. We removed instanceCounter_ since it was never used and moved the dummyVtx function static used as a return value to be a const
class static. The statics associated with setDepthCorParameters and getDepthCorParameters were left as is but they were reported to the appropriate L2s.