MSVS warning C4800 (forcing value to bool) and WebKit headers

[James Robinson]

Chromium code has MSVS warning C4800 and warnings=errors enabled.  C4800 (http://msdn.microsoft.com/en-us/library/b6801kcy(v=vs.71).aspx) warns about coercing a numeric type into a bool.  Chromium style is to compare integers or pointers with 0 or NULL, so it won't typically hit this warning, but WebKit coding style is that all comparisons to 0 / NULL should be done implicitly (http://www.webkit.org/coding/coding-style.html#zero-comparison).

This poses a problem for WebKit API headers that are included into chromium code.  WebKit headers are written in WebKit style, so any header with an inline comparison to 0/NULL will violate C4800 in translation units that include it.  For example a recent patch of mine failed on the win trybots because a WebKit header had code like this:

class WebThing {

public:

    bool isNull() const { return m_thing; }

private:

    Thing* m_thing;

};

Proposals:

1.) Ignore C4800 in targets that include troublesome headers.  We already have this warning ignored in several test_shell targets, but this seems a bit fragile since much of the codebase might end up transitively including WebKit API headers and might be broken by upstream changes.

2.) Ignore C4800 everywhere in chromium. It's not clear what value this is supposed to bring.

3.) Refactor WebKit headers to avoid this warning or violate WebKit style conventions.

I'd lean towards (2) or (1) if people think this warning is valuable.

- James

[Nico Weber]

Unless someone knows examples of where this finds otherwise hard-to-find bugs, I think (2) sounds good. (We can try reenabling the warning a few months down the road and see how much of the things it finds then are bugs, to get an idea how useful the warning is in practice.)

Nico

[Fred Akalin]

I have definitely been bitten by implicit conversions to bool before (most often from pointers).

--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev

[James Robinson]

On Tue, Sep 4, 2012 at 10:32 AM, Fred Akalin <aka...@chromium.org> wrote:

I have definitely been bitten by implicit conversions to bool before (most often from pointers).

Could you define 'bitten'?  What is an example of code that you wrote that you thought was correct but that the warning demonstrated was incorrect?

- James

[Mike Reed]

    bool isNull() const { return m_thing; }

Is it an option to change this to?

    bool isNull() const { return !!m_thing; }

[James Robinson]

On Tue, Sep 4, 2012 at 10:34 AM, Mike Reed <re...@google.com> wrote:

    bool isNull() const { return m_thing; }

Is it an option to change this to?

    bool isNull() const { return !!m_thing; }

Changing the WebKit header to be inconsistent with WebKit style and the rest of the WebKit codebase is option (3).   I don't think this is a really good idea.

- James

[Peter Kasting]

I'd prefer (3), or really, I'd prefer that WebKit style change, this warning be turned on for them, and all violations fixed.  That seems like a pipedream (I can't imagine the Apple folks accepting such a change), so given that, (2) is probably the only feasible option.

PK

[Dominic Mazzoni]

Just throwing out another alternative, with no opinion as to whether
it's better or worse:

4) Disable this warning only for WebKit headers - like using some
macro that would expand to:

#pragma warning(push)
#pragma warning(disable: 4800)
#include "third_party/WebKit/Source/WebKit/chromium/public/WebNode.h"
#pragma warning(pop)

- Dominic

[Fred Akalin]

On Tue, Sep 4, 2012 at 10:34 AM, James Robinson <jam...@google.com> wrote:

On Tue, Sep 4, 2012 at 10:32 AM, Fred Akalin <aka...@chromium.org> wrote:

I have definitely been bitten by implicit conversions to bool before (most often from pointers).

Could you define 'bitten'?  What is an example of code that you wrote that you thought was correct but that the warning demonstrated was incorrect?

I have introduced bugs which would have been caught by this warning.  Working with pointers to bool is especially fraught with danger, as it's easy to write "bool_ptr = false" instead of "*bool_ptr = false", etc.

This warning does produce a lot of false-positives, though.

[Nico Weber]

http://msdn.microsoft.com/en-us/library/b6801kcy(v=vs.71).aspx sounds like the warning is on converting values _to_ bools, so from the description it sounds like it wouldn't catch "bool_ptr = false" (?)

[James Robinson]

Right, it wouldn't help.  It might help if you were to write "bool foo = bool_ptr" when you meant "bool foo = *bool_ptr", but that seems more contrived and something more likely to be caught by good variable names and review.

C4800 is just about conversion to bool.  A few people more familiar with win32 history suggest that this might exist to try to catch mixups of BOOL which is typedef'd to int and bool.  We do seem to have a lot of !!Foo() expressions where Foo() is a win32 API that returns BOOL for this reason.

So far, it seems that (2) is the most appealing option.  I personally like Dominic's idea a lot in theory but can't think of a good practical way to implement it that doesn't make include sites too awkward.

- James

[Darin Fisher]

Has this warning always been enabled?  Or, is it new with VS2010?

I assume it doesn't impact expressions like "if (foo_ptr) {..."?

-Darin

 

- James

 

This warning does produce a lot of false-positives, though.

[Piotr Kufel]

Has this warning always been enabled?  Or, is it new with VS2010?

I recall it was present in 2k8 too. 

I assume it doesn't impact expressions like "if (foo_ptr) {..."?

AFAIR it's a performance warning: I think C++ semantics require bool to be stored as 0 or 1, so (int)(bool)2==1 - it introduces compare/branch in a simple integral type cast. In case of if (expr) you don't have a way to access the boolean value of expr later, so there's no warning because the compiler can just test for expr being nonzero without physically storing the result in one-byte 0/1 value.

[John Bates]

On Wed, Sep 5, 2012 at 1:42 AM, Piotr Kufel <qf...@google.com> wrote:
>
>> Has this warning always been enabled? Or, is it new with VS2010?
>
>
> I recall it was present in 2k8 too.
>
>> I assume it doesn't impact expressions like "if (foo_ptr) {..."?
>
>
> AFAIR it's a performance warning: I think C++ semantics require bool to be
> stored as 0 or 1

Note: the spec does not specify how bool is stored in memory, but when
cast to int, the spec says false becomes 0 and true becomes 1.

> , so (int)(bool)2==1 - it introduces compare/branch in a
> simple integral type cast.

Most CPUs have a select instruction, so I would be surprised to see a
branch. Have you seen this code become a branch in practice?

> In case of if (expr) you don't have a way to
> access the boolean value of expr later, so there's no warning because the
> compiler can just test for expr being nonzero without physically storing the
> result in one-byte 0/1 value.
>

[Piotr Kufel]

I never looked into a disassembly, if there's such instruction then I
guess the compiler would use it. Perhaps it's still significantly more
expensive then other integrals conversions and hence the warning?

2012/9/5 John Bates <jba...@google.com>:

[Victor Khimenko]

On Wed, Sep 5, 2012 at 8:19 PM, Piotr Kufel <qf...@google.com> wrote:

I never looked into a disassembly, if there's such instruction then I
guess the compiler would use it. Perhaps it's still significantly more
expensive then other integrals conversions and hence the warning?

No, it's very fast and does not break the pipeline. It was introduced in P6 back in 1995 and is supported/used today by most compilers.