-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Fixes to 10 coverity defect reports #480
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
Conversation
merging with root master.
|
Could you resolve the conflicts? |
hist/hist/src/TH1Merger.cxx
Outdated
| for (Int_t i = 0; i < nbentries; i++) | ||
| h2->Fill(hist->fBuffer[3*i + 2], hist->fBuffer[3*i + 3],hist->fBuffer[3*i + 1] ); | ||
| if (auto h2 = dynamic_cast<TH2*>(fH0)){ | ||
| R__ASSERT(h2); |
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 seems like a false positive in coverity, i.e. if h2 is nullptr we will issue a fatal error and abort. The assert in the if-stmt here becomes dead code.
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.
Probably coverity does not know that you will never comeback from R__ASSERT with a nullptr.
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 indeed very likely. Is there a way to teach coverity about the semantic of that macro?
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.
Coverity believes that ::Fatal() might return. As a matter of fact I can simply set gAbortLevel to kFatal + 1 and Fatal() will not abort! I.e. indeed and in principle: Coverity is correct here :-)
For all practical purposes though we should annotate Fatal(); see https://coverity.cern.ch/docs/en/cov_checker_ref.html#killpath
I'll take care of it.
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.
Told Coverity about Fatal() in 7c8286d - that should get rid if this "defect".
hist/hist/src/TH1Merger.cxx
Outdated
| for (Int_t i = 0; i < nbentries; i++) | ||
| h3->Fill(hist->fBuffer[4*i + 2], hist->fBuffer[4*i + 3],hist->fBuffer[4*i + 4], hist->fBuffer[4*i + 1] ); | ||
| if (auto h3 = dynamic_cast<TH3*>(fH0)){ | ||
| R__ASSERT(h3); |
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.
Likewise.
This reverts commit c0cf732.
|
@phsft-bot build |
|
Starting build on |
Fix warning
|
@phsft-bot build |
|
Starting build on |
|
One failure of the system in one cell. |
| Float_t xcoord[n], ycoord[n]; | ||
| for ( double xi = 0; xi < 10.01; xi += 0.2) { | ||
|
|
||
| for ( i = 0; i < 50; ++i) { |
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.
Hm, was it intentional to increment i twice: once in the loop and once in the loop's body (see last line).
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 don't understand these changes too. The old code looked perfectly fine to me
No description provided.