GNOME Bugzilla – Bug 730378
addressbook: Strengthen an assertion to avoid negative bit shifts
Last modified: 2014-11-03 22:19:19 UTC
Patch attached.
Created attachment 276777 [details] [review] addressbook: Strengthen an assertion to avoid negative bit shifts This should never happen in practice, but it is good to be completely explicit in the assertion. Coverity issue: #1214485
Review of attachment 276777 [details] [review]: As mentioned elsewhere, do not use g_assert().
Created attachment 276837 [details] [review] addressbook: Strengthen an assertion to avoid negative bit shifts This should never happen in practice, but it is good to be completely explicit in the assertion. Coverity issue: #1214485
Review of attachment 276837 [details] [review]: Looks good, please commit. Thanks.
Attachment 276837 [details] pushed as 5accb2b - addressbook: Strengthen an assertion to avoid negative bit shifts
Created attachment 289781 [details] [review] addressbook: Strengthen an assertion to avoid negative bit shifts This should never happen in practice, but it is good to be completely explicit in the assertion. Follow up to commit 10d55d18. Coverity issue: #1250458
Another instance of it popped up in Coverity. Patch attached.
Review of attachment 289781 [details] [review]: It only prints a warning and then calls the below code anyway. Are you sure it's sufficient for Coverity to not identify the code as faulty?
(In reply to comment #8) > It only prints a warning and then calls the below code anyway. Are you sure > it's sufficient for Coverity to not identify the code as faulty? IIRC, when I originally posted these patches, I used g_assert(), but then (as per comment #2), you wanted g_warn_if_fail() instead. That seems to be OK for Coverity, but I’m not sure why. In any case, it documents the intent better. I think your reasoning for preferring g_warn_if_fail() over g_assert() was to stop the daemon exploding on a problem, and let it stumble on instead (because the undefined behaviour in this case is probably fine). If you want to use g_assert() instead, I would be more than happy to. :-)
Heh, I'm still not in favour of g_assert(), it surprised me that Coverity is fine with something what actually doesn't prevent the problem. If your tests shows that Coverity is fine with it, then feel free to commit to sources as it is. Thanks.
(In reply to comment #10) > Heh, I'm still not in favour of g_assert(), it surprised me that Coverity is > fine with something what actually doesn't prevent the problem. If your tests > shows that Coverity is fine with it, then feel free to commit to sources as it > is. Thanks. Well, even if it turns out that Coverity isn’t fine with it, having the g_warn_if_fail() there makes it obvious that the error in Coverity is a false positive, and can safely be marked as ignored.
Attachment 289781 [details] pushed as 51e9e69 - addressbook: Strengthen an assertion to avoid negative bit shifts