Try to remove a few LogicErrors and DeadStores in DT/CSC LocalMuon reco #27835
Conversation
|
The code-checks are being triggered in jenkins. |
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-27835/11572
|
|
A new Pull Request was created by @perrotta for master. It involves the following packages: RecoLocalMuon/CSCRecHitD @perrotta, @cmsbuild, @slava77 can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
|
please test |
|
The tests are being triggered in jenkins. |
|
Comparison job queued. |
|
Comparison is ready Comparison Summary:
|
cmsbuild
added
code-checks-pending
comparison-pending
and removed
code-checks-approved
comparison-available
tests-approved
labels
|
@ptcox (for CSC) and @namapane @fcavallo @battibass (for DT): could you please give your green light, or let me know when you have time to do so? |
|
@perrotta, I had a look to DT-related changes, and could not spot anything wrong. |
|
OK by me, as long as the comment and the 't_zero' in the output text,
remain.
Tim
Slava Krutelyov wrote on 8/29/19 16:12:
…
***@***.**** commented on this pull request.
------------------------------------------------------------------------
In RecoLocalMuon/CSCRecHitD/src/CSCMake2DRecHit.cc
<#27835 (comment)>:
> @@ -133,8 +133,7 @@ CSCRecHit2D CSCMake2DRecHit::hitFromStripAndWire(const CSCDetId& id,
}
tpeak = peakTimeFinder_->peakTime(tmax, adcArray, tpeak);
// Just for completeness, the start time of the pulse is 133 ns earlier, according to Stan :)
- float t_zero = tpeak - 133.f;
- LogTrace("CSCRecHit") << "[CSCMake2DRecHit] " << id << " strip=" << centerStrip << ", t_zero=" << t_zero
+ LogTrace("CSCRecHit") << "[CSCMake2DRecHit] " << id << " strip=" << centerStrip << ", t_zero=" << tpeak - 133.f
I don't think that this is a good strategy in general to just expand
the variable inline.
I somehow noticed in the recent few PRs the same feature of variables
used in Log* claimed to be unused by the static analyzer.
(perhaps an induced memory) I seem to recall that we were previously
running the static analyzer with EDM_ML_DEBUG enabled.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#27835?email_source=notifications&email_token=ABGYLHVEEKBEYDEJIALDCFLQG7KNFA5CNFSM4IO7AUGKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCDDSVSA#pullrequestreview-281488072>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABGYLHURJZWTRCFBZE7DOV3QG7KNFANCNFSM4IO7AUGA>.
|
|
+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. @davidlange6, @slava77, @smuzaffar, @fabiocos (and backports should be raised in the release meeting by the corresponding L2) |
cmsbuild
added
fully-signed
reconstruction-approved
and removed
pending-signatures
reconstruction-pending
labels
|
please test |
cmsbuild
added
comparison-pending
tests-pending
and removed
comparison-available
tests-approved
labels
|
The tests are being triggered in jenkins. |
|
Comparison job queued. |
|
Comparison is ready Comparison Summary:
|
|
@perrotta : apologies for the delay, I had a look and it seems OK to me as well. |
|
+1 |
PR description:
As the PR title says
Issues were pointed out by the static analyzer during standard tests
PR validation:
It builds and successfully runs the short matrix
No differences expected in output (excepted perhaps tiny effects due to the numerical precision of the replacement of the constant sqrt(12) with 1/sqrt(12) in CSCMake2DRecHit.cc, which can be reverted anyhow, @ptcox)