> For static analysis with Coverity Prevent, the added assert will clear a defect report and/or allow it to continue parsing to the next possible defect.
Are these being checked manually and determined to be false positives?
If not, then it would be beneficial to not shut up static analysis,
since it actually could be a problem.
If yes, perhaps it would be useful to have a comment explaining why it
is not a false positive, unless the reasoning is often trivial, which
means that the static analyzer isn't doing a very good job.
Also, is the whole concept of having a static analyzer assume that
asserts are true a good idea?
Shouldn't it instead specifically attempt to check whether the
assertions in the code are always true? (and have some other means to
flag false positives, perhaps not involving source modification)
Finally, does the checker provide some easy and license-allowed way of
making the analysis results public? (e.g. by putting up the same web
interface they used for their open source checking demos)
BTW, I just looked at one of the assert commits, and found it actually
_introduces_ a bug.
Look at the assert(attrib_list) added in
706fffbff59be0dc884e1938f1bdf731af1efa3e.
This ends up asserting that the attrib_list in glXCreatePixmap is not NULL.
But the GLX specification says that it can be NULL, and it will usually be.
The memcpy does not crash because when attrib_list is NULL, the length
parameter passed to it is 0, as the code before shows.
Thus, that commit should be reverted, and replaced with either no
change or by surrounding the memcpy with if(attrib_list) or if(i) .
Ideally, we could also mark the if, as well as the if(attrib_list)
above with unlikely() while we are at it.
Are we sure all the other commits like this are correct and actually
flag false positives, as opposed to hiding real bugs?
|
