Rearrange #ifdef STAT_TSB so file var statCount does not appear

[gartung]

This is obviously used for local debugging and file vars are not thread safe.

[cmsbuild]

A new Pull Request was created by @gartung (Patrick Gartung) for CMSSW_7_1_X.

Rearrange #ifdef STAT_TSB so file var statCount does not appear

It involves the following packages:

RecoTracker/CkfPattern
RecoTracker/FinalTrackSelectors
RecoTracker/TrackProducer

@nclopezo, @cmsbuild, @anton-a, @thspeer, @slava77, @Degano can you please review it and eventually sign? Thanks.
@gpetruc, @ghellwig, @GiacomoSguazzoni, @rovere, @cerati this is something you requested to watch as well.
You can sign-off by replying to this message having '+1' in the first line of your reply.
You can reject by replying to this message having '-1' in the first line of your reply.
@nclopezo, @ktf you are the release manager for this.
You can merge this pull request by typing 'merge' in the first line of your comment.

[cmsbuild]

+1
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-2412/128/summary.html

[anton-a]

+1
6d56ee8
jenkins and code inspection

[cmsbuild]

This pull request is fully signed and it will be integrated in one of the next CMSSW_7_1_X IBs unless changes (tests are also fine). @nclopezo, @ktf can you please take care of it?

[ktf]

Is the ifdef-ed code actually used?

[anton-a]

possibly for some testing/debugging - I could not find STAT_TSB defined in standard code

[ktf]

Can we actually find out with the tracking people and remove if not used (or add proper support for this kind of profiling so that it makes sense in multithreading?)

[anton-a]

OK. I will contact them and will follow up.

[ktf]

Ok, looks like this is Vincenzo's code. now that I look at it, what is the problem with the original code?

[gartung]

On 2/13/2014 4:56 AM, Giulio Eulisse wrote:

Ok, looks like this is Vincenzo's code. now that I look at it, what is
the problem with the original code?

—
Reply to this email directly or view it on GitHub
#2412 (comment).

The struct is a non-const file var.

[ktf]

But it does nothing right?

[gartung]

On 2/13/2014 7:19 AM, Giulio Eulisse wrote:

But it does nothing right?

—
Reply to this email directly or view it on GitHub
#2412 (comment).

True. But why leave it in if it does nothing.

[ktf]

Reply to this email directly or view it on GitHub
#2412 (comment).
True. But why leave it in if it does nothing.

Discussing with Vincenzo, he actually uses it for developing tracking
code, so it's a valid use case. Given the code is safe in the
multithreaded case, I would not change it further.

[gartung]

@Dr15Jones can you comment please.

[Dr15Jones]

Having it in confuses the static analyzer. If Patrick has already done the work in #defining it out why not allow it?

[ktf]

Because scattering the code of ifdefs for what is a static analyser
false positive is not a good idea, IMHO. Either we agree that the code
needs to be removed (and the author, Vincenzo, does not) or we clean up
the false positive. Can't we fix it with an attribute like we have done
elsewhere?

On 13 Feb 2014, at 16:02, Chris Jones wrote:

Having it in confuses the static analyzer. If Patrick has already done
the work in #defining it out why not allow it?

---

Reply to this email directly or view it on GitHub:
#2412 (comment)

[gartung]

On 2/13/2014 9:21 AM, Giulio Eulisse wrote:

Because scattering the code of ifdefs for what is a static analyser
false positive is not a good idea, IMHO. Either we agree that the code
needs to be removed (and the author, Vincenzo, does not) or we clean up
the false positive. Can't we fix it with an attribute like we have done
elsewhere?

On 13 Feb 2014, at 16:02, Chris Jones wrote:

Having it in confuses the static analyzer. If Patrick has already done
the work in #defining it out why not allow it?

---

Reply to this email directly or view it on GitHub:
#2412 (comment)

—
Reply to this email directly or view it on GitHub
#2412 (comment).

OK. I will add the [[cms::threadsafe]] attribute instead.

[gartung]

So your concern is esthetics of the code????

[ktf]

My concern is polluting code with something unnecessary. If we can do it with a one line solution rather than a multitude of ifdefs, why not?

[cmsbuild]

Pull request #2412 was updated. @nclopezo, @cmsbuild, @anton-a, @thspeer, @slava77, @Degano can you please check and sign again.

[ktf]

@nclopezo please just check it compiles fine.

[Dr15Jones]

This is misleading. If we are going to do this then it should be

#ifdef STAT_TSB
StatCount statCount;
#else
[[cms::thread_safe]] StatCount statCount;
#endif

That will properly describe the case.

[gartung]

Agreed.

[cmsbuild]

Pull request #2412 was updated. @nclopezo, @cmsbuild, @anton-a, @thspeer, @slava77, @Degano can you please check and sign again.

[cmsbuild]

+1
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-2412/174/summary.html

[ktf]

Fine with me. Can you please squash the commits?

[cmsbuild]

Pull request #2412 was updated. @nclopezo, @cmsbuild, @anton-a, @thspeer, @slava77, @Degano can you please check and sign again.

[gartung]

Squashed.

[ktf]

Thanks. Bypassing this. Complain if not ok.

[cmsbuild]

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-2412/262/summary.html