On Chromium, this found another bug in addition to the one we already knew about, and had 0 false positives. Likely worth it.

Evaluated this on chromium (found another bug, and 0 false positives), wrote tests, and sent the patch out: http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20131216/096073.html

Initial version checked in at r198063.

Might want to try to extend this to all function calls that have a size_t (or unsigned?) in the last parameter?

generalized patch
Here's a prototype of doing this for all calls where the last parameter is an unsigned. I haven't really evaluated this yet (and the patch needs lots of cleanups too).

(Without the char check there lots of false positives in mesa, due to GLboolean being an unsigned char)

yasm defines TRUE as (0==0) and FALSE as (0!=0) leading to all kinds of funny errors:

../../third_party/yasm/source/patched-yasm/libyasm/bitvect.c:1688:38: error: size argument in 'BitVector_Create' call is a comparison [-Werror,-Wmemsize-comparison]
rest = BitVector_Create(bits,FALSE);
^~~~~
../../third_party/yasm/source/patched-yasm/libyasm/bitvect.h:73:23: note: expanded from macro 'FALSE'
#define FALSE (0!=0)
~^~~
../../third_party/yasm/source/patched-yasm/libyasm/bitvect.c:1688:43: note: did you mean to compare the result of 'BitVector_Create' instead?
rest = BitVector_Create(bits,FALSE);
~~~~~~~~~~~~~~~~ ^

We should probably not emit this warning if the comparison is from a macro expansion (probably for the currently-checked in version too) :-D

False positive in xz on:

static inline void
rc_bit(lzma_range_encoder *rc, probability *prob, uint32_t bit) { ... }

	rc_bit(&coder->rc,
			&coder->is_rep0_long[coder->state][pos_state],
			len != 1);

False positive in yasm (many):

    typedef enum boolean { false = FALSE, true = TRUE } boolean;

YASM_LIB_DECL
void BitVector_LSB (/@out@/ wordptr addr, boolean bit);

        BitVector_LSB(result, !BitVector_is_empty(op1) ||
                      !BitVector_is_empty(op2));

…I suppose this shouldn't fire on enums, or at least not on enums that have exactly two values? Going with the former for now.

generalized patch, v2
This is a slightly better version of the previous generalized patch.

Hm, the xz false positive could be fixed by only triggering the warning on functions that don't return void.

generalized patch, v3
With the void check.

The patch still has one false positive I think:

bool MarkAsCompleted(uint64 result);

bool AllSamplesPassedQuery::Process() {
// ...
return MarkAsCompleted(result != 0);
}

and relying on the void check feels brittle (if someone adds a status return code to that xz function, the warning starts firing again).

So the warning is probably better as-is?

It does find one thing that looks like a bug though.

The one thing it found was this bit in nss (which I've reported to nss authors):

In nss/lib/softoken/pkcs11.c around line 1110:

case CKK_NSS_JPAKE_ROUND1:
    if (!sftk_hasAttribute(object, CKA_PRIME ||
        !sftk_hasAttribute(object, CKA_SUBPRIME) ||
        !sftk_hasAttribute(object, CKA_BASE))) {
        return CKR_TEMPLATE_INCOMPLETE;
    }

(Note the missing ')' in the first sftk_hasAttribute call – but it's a harmless bug, I'm told.)

nss bug: https://bugzilla.mozilla.org/show_bug.cgi?id=957727

The MarkAsCompleted() thing is apparently more a bug than a false positive too. Maybe worth running this over more code bases to get a better feel for how it does.