Fixes for re-enabling more MSVC level 4 warnings: mojo/ edition

Fixes for re-enabling more MSVC level 4 warnings: mojo/ edition

This contains fixes for the following sorts of issues:
* Assignment inside conditional
* Signedness mismatch

BUG=81439
TEST=none

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=281964

[jamesr, 2014-07-08 05:03:25]

I think vtl wrote these tests.  Not sure exactly what the intent is, so I'll let
him review.

[viettrungluu, 2014-07-08 15:19:28]

https://codereview.chromium.org/372163005/diff/1/mojo/public/cpp/environment/...
File mojo/public/cpp/environment/tests/logging_unittest.cc (left):

https://codereview.chromium.org/372163005/diff/1/mojo/public/cpp/environment/...
mojo/public/cpp/environment/tests/logging_unittest.cc:346: // Also try to make
sure that we parenthesize the condition properly.
Uhm, no. Way to defeat exactly what we're testing. Not LGTM.

[viettrungluu, 2014-07-08 15:21:18]

On 2014/07/08 15:19:28, viettrungluu wrote:
>
https://codereview.chromium.org/372163005/diff/1/mojo/public/cpp/environment/...
> File mojo/public/cpp/environment/tests/logging_unittest.cc (left):
> 
>
https://codereview.chromium.org/372163005/diff/1/mojo/public/cpp/environment/...
> mojo/public/cpp/environment/tests/logging_unittest.cc:346: // Also try to make
> sure that we parenthesize the condition properly.
> Uhm, no. Way to defeat exactly what we're testing. Not LGTM.

More specifically, you should change this to something else that checks that the
check is parenthesized properly. E.g., you could substitute += (with other
appropriate changes).

[Peter Kasting, 2014-07-08 19:01:20]

On 2014/07/08 15:21:18, viettrungluu wrote:
> On 2014/07/08 15:19:28, viettrungluu wrote:
> >
>
https://codereview.chromium.org/372163005/diff/1/mojo/public/cpp/environment/...
> > File mojo/public/cpp/environment/tests/logging_unittest.cc (left):
> > 
> >
>
https://codereview.chromium.org/372163005/diff/1/mojo/public/cpp/environment/...
> > mojo/public/cpp/environment/tests/logging_unittest.cc:346: // Also try to
make
> > sure that we parenthesize the condition properly.
> > Uhm, no. Way to defeat exactly what we're testing. Not LGTM.
> 
> More specifically, you should change this to something else that checks that
the
> check is parenthesized properly. E.g., you could substitute += (with other
> appropriate changes).

That won't work either.  No form of assignment (=, +=, or anything else) is
allowed within a conditional expression.  It doesn't matter how many parentheses
it has around it, either; the compiler bans such a thing entirely.  (I tested
this before I originally wrote this change.)

What I was trying to say in the change description is that there's no need to
test that this works anymore in any form, because no one will be able to commit
the sort of code that might break.  That's the whole reason why I removed these
pieces of code instead of updating them.

(Besides this it seems extremely unlikely to me that, now that you have macros
which properly parenthesize your args, you need a set of tests that verify that
continues -- are you really going to change the macros at all, let alone to
remove the parens?)

Please reconsider.  If there is some other form of code than an assignment which
will break if the macros don't properly parenthesize their args, I'm happy to
change to that instead of removing the tests.  Perhaps something with a comma
operator?

[viettrungluu, 2014-07-08 21:26:11]

On 2014/07/08 19:01:20, Peter Kasting wrote:
> On 2014/07/08 15:21:18, viettrungluu wrote:
> > On 2014/07/08 15:19:28, viettrungluu wrote:
> > >
> >
>
https://codereview.chromium.org/372163005/diff/1/mojo/public/cpp/environment/...
> > > File mojo/public/cpp/environment/tests/logging_unittest.cc (left):
> > > 
> > >
> >
>
https://codereview.chromium.org/372163005/diff/1/mojo/public/cpp/environment/...
> > > mojo/public/cpp/environment/tests/logging_unittest.cc:346: // Also try to
> make
> > > sure that we parenthesize the condition properly.
> > > Uhm, no. Way to defeat exactly what we're testing. Not LGTM.
> > 
> > More specifically, you should change this to something else that checks that
> the
> > check is parenthesized properly. E.g., you could substitute += (with other
> > appropriate changes).
> 
> That won't work either.  No form of assignment (=, +=, or anything else) is
> allowed within a conditional expression.  It doesn't matter how many
parentheses
> it has around it, either; the compiler bans such a thing entirely.  (I tested
> this before I originally wrote this change.)
> 
> What I was trying to say in the change description is that there's no need to
> test that this works anymore in any form, because no one will be able to
commit
> the sort of code that might break.  That's the whole reason why I removed
these
> pieces of code instead of updating them.

This is untrue. Not all code compiles on Windows. Moreover, mojo/public is
intended to be (and will be) used outside of Chromium proper (e.g., in Chrome OS
code).

> 
> (Besides this it seems extremely unlikely to me that, now that you have macros
> which properly parenthesize your args, you need a set of tests that verify
that
> continues -- are you really going to change the macros at all, let alone to
> remove the parens?)

It means that refactoring the code is hazardous and hard to do.

> 
> Please reconsider.  If there is some other form of code than an assignment
which
> will break if the macros don't properly parenthesize their args, I'm happy to
> change to that instead of removing the tests.  Perhaps something with a comma
> operator?

Hmmm, commas don't really play well with macro arguments, so I don't know how
that'd work. Possibly you could come up with something involving ?:.

That said, I'd be okay with adding something like

#if defined(_MSC_VER)
#pragma warning(disable:...)
#endif

somewhere in the file (you can't depend on base/compiler_specific.h there, so
the manual ifdef/pragma will have to do).

[Peter Kasting, 2014-07-08 23:04:04]

On 2014/07/08 21:26:11, viettrungluu wrote:
> Hmmm, commas don't really play well with macro arguments, so I don't know how
> that'd work. Possibly you could come up with something involving ?:.

I found alternate formulations for these tests that don't involve assignment.

The tests using the .* operator will fail to compile if the macros in question
lose the parens around the argument.

The tests using "false || x" will fail to pass if the macros in question lose
the parens.  (In the case of MOJO_DCHECK, it will fail in release mode.)

PTAL.

[viettrungluu, 2014-07-08 23:16:04]

https://codereview.chromium.org/372163005/diff/20001/mojo/public/cpp/environm...
File mojo/public/cpp/environment/tests/logging_unittest.cc (right):

https://codereview.chromium.org/372163005/diff/20001/mojo/public/cpp/environm...
mojo/public/cpp/environment/tests/logging_unittest.cc:432: // |MOJO_DCHECK()|
should compile (but not evaluate) its condition even for
So for this, it's intentionally divergent between debug and non-debug builds. I
somehow managed to forget the EXPECT_TRUE(x); in the NDEBUG case and the
EXPECT_FALSE(x); in the !NDEBUG case. :/

For release builds, you want to know that what's inside is not evaluated
(whereas for debug builds it is evaluated).

You could do this cleanly with something like:

bool DcheckTestHelper(bool* was_called) {
  *was_called = true;
  return false;
}

...

  bool was_called = false;
  MOJO_DCHECK(DcheckTestHelper(&was_called)) << "hello";
  ...

[Peter Kasting, 2014-07-08 23:59:54]

https://codereview.chromium.org/372163005/diff/20001/mojo/public/cpp/environm...
File mojo/public/cpp/environment/tests/logging_unittest.cc (right):

https://codereview.chromium.org/372163005/diff/20001/mojo/public/cpp/environm...
mojo/public/cpp/environment/tests/logging_unittest.cc:432: // |MOJO_DCHECK()|
should compile (but not evaluate) its condition even for
On 2014/07/08 23:16:04, viettrungluu wrote:
> So for this, it's intentionally divergent between debug and non-debug builds.
I
> somehow managed to forget the EXPECT_TRUE(x); in the NDEBUG case and the
> EXPECT_FALSE(x); in the !NDEBUG case. :/
> 
> For release builds, you want to know that what's inside is not evaluated
> (whereas for debug builds it is evaluated).
> 
> You could do this cleanly with something like:
> 
> bool DcheckTestHelper(bool* was_called) {
>   *was_called = true;
>   return false;
> }
> 
> ...
> 
>   bool was_called = false;
>   MOJO_DCHECK(DcheckTestHelper(&was_called)) << "hello";
>   ...

Done.

[viettrungluu, 2014-07-09 00:41:29]

LGTM. Thanks a lot for doing this!

[Peter Kasting, 2014-07-09 00:45:51]

The CQ bit was checked by pkasting@chromium.org

[commit-bot: I haz the power, 2014-07-09 00:47:32]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/pkasting@chromium.org/372163005/60001

[commit-bot: I haz the power, 2014-07-09 05:51:47]

Change committed as 281964