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

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

This contains fixes for the following sorts of issues:
* Assignment inside conditional
* Possibly-uninitialized local variable
* Signedness mismatch

This also contains a small number of other cleanups/simplifications to
nearby code.

BUG=81439
TEST=none

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

[Peter Kasting, 2014-07-09 18:55:19]

https://codereview.chromium.org/382613002/diff/1/sandbox/win/src/handle_dispa...
File sandbox/win/src/handle_dispatcher.cc (right):

https://codereview.chromium.org/382613002/diff/1/sandbox/win/src/handle_dispa...
sandbox/win/src/handle_dispatcher.cc:70: ipc->return_info.nt_status = error;
Note that because win32_result and nt_status are in a union, this doesn't
actually result in the sort of behavior change it might look like.  (Similar
comments apply elsewhere in this CL; in general, changes like this are attempts
to make sure we read and write the fields that match the types of the local
variables we're reading and writing to.)

[cpu_(ooo_6.6-7.5), 2014-07-10 00:49:35]

nice cleanup, thanks.

lgtm with the following comments

https://codereview.chromium.org/382613002/diff/1/sandbox/win/src/handle_dispa...
File sandbox/win/src/handle_dispatcher.cc (right):

https://codereview.chromium.org/382613002/diff/1/sandbox/win/src/handle_dispa...
sandbox/win/src/handle_dispatcher.cc:70: ipc->return_info.nt_status = error;
yeah that reads like it was a bug, but it can become one easily if one would
remove the union which its an optimization if my memory is correct.

https://codereview.chromium.org/382613002/diff/1/sandbox/win/src/process_poli...
File sandbox/win/src/process_policy_test.cc (right):

https://codereview.chromium.org/382613002/diff/1/sandbox/win/src/process_poli...
sandbox/win/src/process_policy_test.cc:165: if
(!::SetCurrentDirectory(system32.c_str()))
you are using the nice no brackets style for if() of a single statement here but
it seems that this is not the style of this file.

You can 1) follow the style of the file or 2) change the style of the file. If
you choose 2) please do a single change with that and tbr that if you want.

https://codereview.chromium.org/382613002/diff/1/sandbox/win/src/sync_dispatc...
File sandbox/win/src/sync_dispatcher.cc (right):

https://codereview.chromium.org/382613002/diff/1/sandbox/win/src/sync_dispatc...
sandbox/win/src/sync_dispatcher.cc:57: result, *ipc->client_info, *name,
event_type, initial_state, &handle);
don't you need 4 spaces before result?

[Peter Kasting, 2014-07-10 01:19:31]

(New patch not up yet because I ran out of time, will upload tomorrow; in the
meantime see first comment below)

https://codereview.chromium.org/382613002/diff/1/sandbox/win/src/handle_dispa...
File sandbox/win/src/handle_dispatcher.cc (right):

https://codereview.chromium.org/382613002/diff/1/sandbox/win/src/handle_dispa...
sandbox/win/src/handle_dispatcher.cc:70: ipc->return_info.nt_status = error;
On 2014/07/10 00:49:34, cpu wrote:
> yeah that reads like it was a bug, but it can become one easily if one would
> remove the union which its an optimization if my memory is correct.

I don't know if I understand this comment.

As far as I can tell, it was already the case that we sometimes wrote one format
and then read the other.  In other words, the union is not just for space
savings, it's actually functionally necessary (it effectively casts types back
and forth).

Given that, I don't know how to interpret what you're saying I should be doing
here.

https://codereview.chromium.org/382613002/diff/1/sandbox/win/src/process_poli...
File sandbox/win/src/process_policy_test.cc (right):

https://codereview.chromium.org/382613002/diff/1/sandbox/win/src/process_poli...
sandbox/win/src/process_policy_test.cc:165: if
(!::SetCurrentDirectory(system32.c_str()))
On 2014/07/10 00:49:34, cpu wrote:
> you are using the nice no brackets style for if() of a single statement here
but
> it seems that this is not the style of this file.

Good catch, I didn't look hard enough.

https://codereview.chromium.org/382613002/diff/1/sandbox/win/src/sync_dispatc...
File sandbox/win/src/sync_dispatcher.cc (right):

https://codereview.chromium.org/382613002/diff/1/sandbox/win/src/sync_dispatc...
sandbox/win/src/sync_dispatcher.cc:57: result, *ipc->client_info, *name,
event_type, initial_state, &handle);
On 2014/07/10 00:49:34, cpu wrote:
> don't you need 4 spaces before result?

Yes.  Oops.

[Peter Kasting, 2014-07-10 18:17:42]

New snap up.  I also fixed a few Chromium style guide violations (no else after
return, prefer foo == 0 to 0 == foo) while I was touching one of the files.

Waiting to commit until you respond to my previous question.

[cpu_(ooo_6.6-7.5), 2014-07-10 21:37:17]

there was no action required in my comment. Just an observation.

[Peter Kasting, 2014-07-10 21:38:08]

The CQ bit was checked by pkasting@chromium.org

[Peter Kasting, 2014-07-10 21:38:35]

On 2014/07/10 21:37:17, cpu wrote:
> there was no action required in my comment. Just an observation.

OK.  I still don't fully understand what you were observing.  But moving forward
with CQ+ :)

[cpu_(ooo_6.6-7.5), 2014-07-10 21:39:27]

lgtm on the new style changes as well.

[commit-bot: I haz the power, 2014-07-10 21:41:13]

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

[commit-bot: I haz the power, 2014-07-10 22:56:25]

Change committed as 282451