-
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
Properly handle arrays where index can be negative #21151
Conversation
Instead of relying on remembering to always adding an offset to the index being passed to the array, we now use a helper class which always does the offsetting for us. The helper also properly initializes the size of the array to be able to contain the min and max bounds.
The code-checks are being triggered in jenkins. |
@davidlange6 @smuzaffar why does this show up as new-package-pending when the code I changed is already in the release? |
+code-checks |
A new Pull Request was created by @Dr15Jones (Chris Jones) for master. It involves the following packages: L1Trigger/L1TTwinMux The following packages do not have a category, yet: L1Trigger/L1TTwinMux @cmsbuild can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
assign l1 |
The package was never added to https://github.com/cms-sw/cms-bot/blob/master/categories.py |
add L1Trigger/L1TTwinMux under L1 category ( cms-sw/cmssw#21151 )
@Dr15Jones , L1Trigger/L1TTwinMux is now added under l1. |
please test |
The tests are being triggered in jenkins. |
@smuzaffar Thanks! |
+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:
|
The only difference is in the number of MessageLogger Errors and Warnings (I think they decreased) in the workflow 10224.0. |
@davidlange6 this should fix the crashes in the aarch64 crashes in the IBs. |
@@ -69,12 +93,12 @@ void RPCHitCleaner::run(const edm::EventSetup& c) { | |||
for( auto chamber = m_inrpcDigis.begin(); chamber != m_inrpcDigis.end(); ++chamber ){ | |||
RPCDetId detid = (*chamber).first; | |||
if(detid.region()!=0 ) continue; //Region = 0 Barrel | |||
int strips[5] = {0}; | |||
BxToStrips strips; |
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 @Dr15Jones @rekovic - so I guess the real bug here is that either strips[5] should have been strips[7] (3+3+1=7) or that fabs(digi->bx())>3 should have been >=3 (so that 2+2+1=5). @rekovic - can you confirm which one it is sometime today?
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 first bug I found was this one
https://github.com/cms-sw/cmssw/pull/21151/files#diff-bb4facdb289f9efc85f12aad556325e7R124
It was a missing ‘+3’ in the array index calculation.
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.
Ignore that, I shouldn’t reply to messages before having coffee in the morning.
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.
seems @rekovic won't comment so we'll go forward here. (and push in 94x)
I ran the failing wf's on arm with this changes and I confirm this PR is fixing most of the fails (maybe all) |
merge |
Instead of relying on remembering to always adding an offset to the
index being passed to the array, we now use a helper class which
always does the offsetting for us. The helper also properly initializes
the size of the array to be able to contain the min and max bounds.