Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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'm worried about this change:
Human inspection of the code here is made very difficult because a 2-character variable name (
it
) that is also a common english word is used over the scope of 451 lines. I decided that's the first thing we need to fix:#4817
With that it's much easier to see that Coverty might have a point, therefore I also added
assert(curr_overl != nullptr);
in the only place where that condition could potentially false (unless I'm missing something, plmk).Could you please run Coverty for PR #4817? To make this easy, you could simply insert
assert(it != nullptr);
in this PR and undo your change here. Is Coverty happy with that?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.
Let me try and report back, hoping for early next week.
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 reran the Coverity scan for my project using pybind11 source from #4817, the issue is still there
If you feel like this issue is a false positive, please feel free to close this PR.
I would keep changes in #4817 though. Does make code easier to read.
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.
Thanks for trying.
Yes I believe it's a false positive, but I'd like to try a little more if we can silence it in a reasonable way, with a reasonably small effort.
Two ideas/questions:
assert()
? I guess there could be a rationale for ignoring it, not sure.assert()
also right before Coverty thinks there is a problem (your latest screenshot)? I.e.:Does that help?
If not, could you please try this instead of that
assert()
?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 ignores assert because I am building my project in non-debug mode. Let me try adding the suggested error check with raising of internal error. It should work.
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 can confirm that applying the following change:
to code in #4817 resolves the Coverity issue.
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.
That's great to know, thanks!
Is it possible to run Coverty without
NDEBUG
defined? That would be best IMO. Then we could use the conciseassert()
and it wouldn't impact production builds 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.
Yes, running Coverity analysis on release build with
-O3 -UNDEBUG
the issue also disappears.