Cleanup: avoid foo ? true : false, part 2.

Cleanup: avoid foo ? true : false, part 2.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=157509

[Lei Zhang, 2012-09-18 18:27:33]

rvargas: net/ and sandbox/
dmichael: ppapi/

[dmichael (off chromium), 2012-09-18 18:32:26]

lgtm

Seems fine. Just curious...  why do we care? (non_bool_expression ? true :
false) seems okay to me, for converting an integral to a bool.

[Lei Zhang, 2012-09-18 18:40:12]

On 2012/09/18 18:32:26, dmichael wrote:
> lgtm
> 
> Seems fine. Just curious...  why do we care? (non_bool_expression ? true :
> false) seems okay to me, for converting an integral to a bool.

The ones that are really bad style wise are the "(foo == bar) ? true : false;"
There's really no need to write out "? true : false" in many cases.

[rvargas (doing something else), 2012-09-18 18:56:28]

lgtm,

Most of the ones that I looked at were actually ok by that logic (you actually
had to add a != 0)

http://codereview.chromium.org/10942004/diff/6002/sandbox/win/src/sandbox_nt_...
File sandbox/win/src/sandbox_nt_util.cc (left):

http://codereview.chromium.org/10942004/diff/6002/sandbox/win/src/sandbox_nt_...
sandbox/win/src/sandbox_nt_util.cc:16: }
I actually prefer if the anonymous namespace is not part of the sandbox
namespace.

http://codereview.chromium.org/10942004/diff/6002/sandbox/win/src/sandbox_uti...
File sandbox/win/src/sandbox_utils.cc (right):

http://codereview.chromium.org/10942004/diff/6002/sandbox/win/src/sandbox_uti...
sandbox/win/src/sandbox_utils.cc:30: return !!get_module_handle_ex(flags,
module_name, module);
This one I actually dislike a lot. how about xx != FALSE

[Lei Zhang, 2012-09-19 01:32:41]

https://chromiumcodereview.appspot.com/10942004/diff/6002/sandbox/win/src/san...
File sandbox/win/src/sandbox_nt_util.cc (left):

https://chromiumcodereview.appspot.com/10942004/diff/6002/sandbox/win/src/san...
sandbox/win/src/sandbox_nt_util.cc:16: }
On 2012/09/18 18:56:28, rvargas wrote:
> I actually prefer if the anonymous namespace is not part of the sandbox
> namespace.

Done.

https://chromiumcodereview.appspot.com/10942004/diff/6002/sandbox/win/src/san...
File sandbox/win/src/sandbox_utils.cc (right):

https://chromiumcodereview.appspot.com/10942004/diff/6002/sandbox/win/src/san...
sandbox/win/src/sandbox_utils.cc:30: return !!get_module_handle_ex(flags,
module_name, module);
On 2012/09/18 18:56:28, rvargas wrote:
> This one I actually dislike a lot. how about xx != FALSE

Done.

[commit-bot: I haz the power, 2012-09-19 01:32:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/thestig@chromium.org/10942004/14001

[commit-bot: I haz the power, 2012-09-19 05:29:00]

Change committed as 157509

[Peter Kasting, 2012-09-19 21:26:31]

http://codereview.chromium.org/10942004/diff/6002/sandbox/win/src/sandbox_uti...
File sandbox/win/src/sandbox_utils.cc (right):

http://codereview.chromium.org/10942004/diff/6002/sandbox/win/src/sandbox_uti...
sandbox/win/src/sandbox_utils.cc:30: return !!get_module_handle_ex(flags,
module_name, module);
On 2012/09/19 01:32:41, Lei Zhang wrote:
> On 2012/09/18 18:56:28, rvargas wrote:
> > This one I actually dislike a lot. how about xx != FALSE
> 
> Done.

We shouldn't need the "!!" or the "!= FALSE", should we?  Chrome style for a
long time has been to treat false and FALSE as equivalent.

[rvargas (doing something else), 2012-09-19 21:35:13]

http://codereview.chromium.org/10942004/diff/6002/sandbox/win/src/sandbox_uti...
File sandbox/win/src/sandbox_utils.cc (right):

http://codereview.chromium.org/10942004/diff/6002/sandbox/win/src/sandbox_uti...
sandbox/win/src/sandbox_utils.cc:30: return !!get_module_handle_ex(flags,
module_name, module);
On 2012/09/19 21:26:31, Peter Kasting wrote:
> On 2012/09/19 01:32:41, Lei Zhang wrote:
> > On 2012/09/18 18:56:28, rvargas wrote:
> > > This one I actually dislike a lot. how about xx != FALSE
> > 
> > Done.
> 
> We shouldn't need the "!!" or the "!= FALSE", should we?  Chrome style for a
> long time has been to treat false and FALSE as equivalent.

There's the annoying "performance" warning of VS about automatically casting int
to bool :(

I'd be in favor of disabling that warning globally.

[Peter Kasting, 2012-09-19 21:37:40]

On 2012/09/19 21:35:13, rvargas wrote:
>
http://codereview.chromium.org/10942004/diff/6002/sandbox/win/src/sandbox_uti...
> File sandbox/win/src/sandbox_utils.cc (right):
> 
>
http://codereview.chromium.org/10942004/diff/6002/sandbox/win/src/sandbox_uti...
> sandbox/win/src/sandbox_utils.cc:30: return !!get_module_handle_ex(flags,
> module_name, module);
> On 2012/09/19 21:26:31, Peter Kasting wrote:
> > On 2012/09/19 01:32:41, Lei Zhang wrote:
> > > On 2012/09/18 18:56:28, rvargas wrote:
> > > > This one I actually dislike a lot. how about xx != FALSE
> > > 
> > > Done.
> > 
> > We shouldn't need the "!!" or the "!= FALSE", should we?  Chrome style for a
> > long time has been to treat false and FALSE as equivalent.
> 
> There's the annoying "performance" warning of VS about automatically casting
int
> to bool :(

My point was, I thought that particular warning didn't fire for this conversion.
 Maybe I'm mistaken though.

[Lei Zhang, 2012-09-19 21:42:29]

On 2012/09/19 21:37:40, Peter Kasting wrote:
> On 2012/09/19 21:35:13, rvargas wrote:
> >
>
http://codereview.chromium.org/10942004/diff/6002/sandbox/win/src/sandbox_uti...
> > File sandbox/win/src/sandbox_utils.cc (right):
> > 
> >
>
http://codereview.chromium.org/10942004/diff/6002/sandbox/win/src/sandbox_uti...
> > sandbox/win/src/sandbox_utils.cc:30: return !!get_module_handle_ex(flags,
> > module_name, module);
> > On 2012/09/19 21:26:31, Peter Kasting wrote:
> > > On 2012/09/19 01:32:41, Lei Zhang wrote:
> > > > On 2012/09/18 18:56:28, rvargas wrote:
> > > > > This one I actually dislike a lot. how about xx != FALSE
> > > > 
> > > > Done.
> > > 
> > > We shouldn't need the "!!" or the "!= FALSE", should we?  Chrome style for
a
> > > long time has been to treat false and FALSE as equivalent.
> > 
> > There's the annoying "performance" warning of VS about automatically casting
> int
> > to bool :(
> 
> My point was, I thought that particular warning didn't fire for this
conversion.
>  Maybe I'm mistaken though.

If you are on Windows, you can probably find out a lot faster than I can.

BTW, sandbox_utils.h says the reason we have GetModuleHandleHelper() is for
compatibility with Win2K. Isn't it time to get rid of GetModuleHandleHelper
altogether and just call GetModuleHandleEx() directly?

[Peter Kasting, 2012-09-19 22:55:17]

On 2012/09/19 21:42:29, Lei Zhang wrote:
> BTW, sandbox_utils.h says the reason we have GetModuleHandleHelper() is for
> compatibility with Win2K. Isn't it time to get rid of GetModuleHandleHelper
> altogether and just call GetModuleHandleEx() directly?

I think so.  I'll take care of this.  Good catch.