[cfe-commits] [Patch] Checker for assignment of non-Boolean value to Boolean variable

[David Blaikie]

On Thu, Dec 8, 2011 at 10:23 PM, Anna Zaks <ganna at apple.com> wrote:

>
> On Dec 8, 2011, at 9:21 PM, David Blaikie wrote:
>
>
>
> On Thu, Dec 8, 2011 at 9:14 PM, Anna Zaks <ganna at apple.com> wrote:
>
>>
>> On Dec 8, 2011, at 6:23 PM, David Blaikie wrote:
>>
>> > Yeah - now I understand the problem, I wonder again if this could be
>> > implemented (in a simpler but more strict manner) as a front-end
>> > warning in the same way that null-conversions are warned about (though
>> > it'd have to be a bit smarter - perhaps detecting the macro/typedef
>> > that was used to declare a variable or assign a constant to it - not
>> > to mention that the current Clang null-conversion support is broken
>> > (I've another thread pending some discussion on that)). But in general
>> > this should be good for any >2 state value masquerading as an int -
>> > for C++ bool or anything that's as good as it, a lesser warning (if
>> > any at all) might be appropriate.
>> >
>>
>> My understanding is that you are suggesting to add a compiler warning
>> when a constant, which is not 0 nor 1, is being assigned to a boolean
>> variable. Such warning would be weaker then this checker, so the checker
>> should still go in.
>>
>
> I'm certainly not sufficiently informed to make any claim to veto this
> checker - just tossing around some ideas.
>
> I assume the "weakness" you're referring to, based on the compiler warning
> you described is that it wouldn't catch this:
>
> unsigned i = 3;
> BOOL j = i;
>
>
> Not really. I can imagine this case being handled by the compiler. Ex:
> seems straightforward for constant propagation(though we probably do not
> have that in the front end).
>

Sorry, should've provided a more compelling example (& yes, the frontend
has some amount of constant propagation, but it's fairly simplistic -
doesn't infer through conditions or anything fancy like that, so far as I
know).


> The static analyzer can handle much more interesting cases (in addition to
> this one). Here is how most of the regression tests in the attached patch
> look like:
>
> +void test_cppbool_initialization(int y) {
> +  if (y < 0) {
> +    bool x = y; // expected-warning {{Assignment of a non-Boolean value}}
> +    return;
> +  }
> +  if (y > 1) {
> +    bool x = y; // expected-warning {{Assignment of a non-Boolean value}}
> +    return;
> +  }
> +  bool x = y; // no-warning
> +}
>

What I was suggesting was something a little, for what of a better term,
dumber. Warning for any assignment from non-BOOL to BOOL. As I mentioned,
perhaps this would be too noisy for existing code (I don't know enough
about how average code uses these types to speak from any particular
experience) but plausible for new code (& more immediate than running the
static analyzer) - though perhaps even for new code this would be too
restrictive, breaking common idioms, etc - I don't know.

But, yes, this seems valuable (higher accuracy than what I was suggesting -
at the very least for legacy code it should be helpful, and my suggestion
might be even too restrictive for new code) for pseudo-bool types (that are
really multibit integer values capable of accidentally storing values other
than 0 or 1 quite easily) but probably wrong for C++ true 'bool'.

- David


>
>
> That could be dealt with by checking that all assignments to/from BOOL (or
> whatever other pseuod bool types exist) are from other BOOLs - this might
> be somewhat noisy, I agree - the checker would be more surgical though the
> general warning might suffice, especially for new code.
>
> This might still be too expensive, I don't know - looking up function &
> variable declarations whenever a BOOL expression is encountered to see how
> the name was spelled. In which case something more like GNU's __null could
> be used - a custom, hidden type that looks just like char or whatever the
> intended underlying type is.
>
>
> I am not sure exactly what you are proposing. Sounds similar to type
> checking and disallowing implicit casts even from int, which is probably
> too noisy for a warning (and even for a checker).
>
> - David
>
>
>>
>> Cheers,
>> Anna.
>>
>> > The usual warning with Win32 BOOL is to never mix BOOL & bool - but
>> > with this kind of warning/checking you could be confident that BOOL is
>> > only 0 or 1 and so "someBOOL == some_bool" would work sensibly (&
>> > never be "2 == 1" => false, for example)
>> >
>> > - David
>> >
>> > _______________________________________________
>> > cfe-commits mailing list
>> > cfe-commits at cs.uiuc.edu
>> > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20111208/bdf62a20/attachment.html>