Fixes for re-enabling more MSVC level 4 warnings: misc edition #2

Fixes for re-enabling more MSVC level 4 warnings: misc edition #2

This contains fixes for the following sorts of issues:
* Assignment inside conditional
* Taking the address of a temporary
* Octal escape sequence terminated by decimal number
* Signedness mismatch
* Possibly-uninitialized local variable

This also contains a small number of cleanups to nearby code (e.g. no else after return).

BUG=81439
TEST=none

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

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

zea: google_apis/gcm/engine/mcs_client.cc
senorblanco: skia/ext/pixel_ref_utils_unittest.cc
darin: rest

https://codereview.chromium.org/382673002/diff/1/google_apis/gcm/engine/mcs_c...
File google_apis/gcm/engine/mcs_client.cc (right):

https://codereview.chromium.org/382673002/diff/1/google_apis/gcm/engine/mcs_c...
google_apis/gcm/engine/mcs_client.cc:774: StreamId last_stream_id_received =
0xFFFFFFFF;
This needs review.  See how code below tries to check whether other values are
greater than this.

https://codereview.chromium.org/382673002/diff/1/skia/ext/pixel_ref_utils_uni...
File skia/ext/pixel_ref_utils_unittest.cc (right):

https://codereview.chromium.org/382673002/diff/1/skia/ext/pixel_ref_utils_uni...
skia/ext/pixel_ref_utils_unittest.cc:39: reader->skip(static_cast<size_t>(-4)); 
//!!? Is this safe?
This needs review.  Static-casting a negative value to an unsigned one and then
actually using it like a negative value seems dangerous to me (means we have
code relying on integer overflow); it suggests the signature of skip() shouldn't
be taking an unsigned value if we want it to be able to move backwards.

https://codereview.chromium.org/382673002/diff/1/tools/json_to_struct/json_to...
File tools/json_to_struct/json_to_struct.py (right):

https://codereview.chromium.org/382673002/diff/1/tools/json_to_struct/json_to...
tools/json_to_struct/json_to_struct.py:108: f.write('\n')
This just ensures the generated code puts all the #includes below the header
guard.

https://codereview.chromium.org/382673002/diff/1/win8/metro_driver/ime/text_s...
File win8/metro_driver/ime/text_store.cc (left):

https://codereview.chromium.org/382673002/diff/1/win8/metro_driver/ime/text_s...
win8/metro_driver/ime/text_store.cc:780: bool has_display_attribute = false;
This variable was never checked below, so we could try and read
display_attribute.fBoldLine without having ever set it.

[Nicolas Zea, 2014-07-09 22:52:35]

https://codereview.chromium.org/382673002/diff/1/google_apis/gcm/engine/mcs_c...
File google_apis/gcm/engine/mcs_client.cc (right):

https://codereview.chromium.org/382673002/diff/1/google_apis/gcm/engine/mcs_c...
google_apis/gcm/engine/mcs_client.cc:774: StreamId last_stream_id_received =
0xFFFFFFFF;
On 2014/07/09 22:19:54, Peter Kasting wrote:
> This needs review.  See how code below tries to check whether other values are
> greater than this.

You're right, this is broken. The lines at 815 are effectively dead code at the
moment. I suspect the only effect this has though is one of inefficiency.

And yes, the right solution here is to set the value to 0. I've filed
crbug.com/392664 to better test this code (looks like it wasn't verifying the
internal ack state properly).

[Peter Kasting, 2014-07-09 23:34:25]

https://codereview.chromium.org/382673002/diff/1/google_apis/gcm/engine/mcs_c...
File google_apis/gcm/engine/mcs_client.cc (right):

https://codereview.chromium.org/382673002/diff/1/google_apis/gcm/engine/mcs_c...
google_apis/gcm/engine/mcs_client.cc:774: StreamId last_stream_id_received =
0xFFFFFFFF;
On 2014/07/09 22:52:35, Nicolas Zea wrote:
> And yes, the right solution here is to set the value to 0.

Done.

[Nicolas Zea, 2014-07-10 00:58:28]

gcm LGTM

[Stephen White, 2014-07-10 02:13:56]

https://codereview.chromium.org/382673002/diff/1/skia/ext/pixel_ref_utils_uni...
File skia/ext/pixel_ref_utils_unittest.cc (right):

https://codereview.chromium.org/382673002/diff/1/skia/ext/pixel_ref_utils_uni...
skia/ext/pixel_ref_utils_unittest.cc:39: reader->skip(static_cast<size_t>(-4)); 
//!!? Is this safe?
On 2014/07/09 22:19:54, Peter Kasting wrote:
> This needs review.  Static-casting a negative value to an unsigned one and
then
> actually using it like a negative value seems dangerous to me (means we have
> code relying on integer overflow); it suggests the signature of skip()
shouldn't
> be taking an unsigned value if we want it to be able to move backwards.

+vmpstr +junov

Much of this code looks dubious. SkOrderedReadBuffer is in Skia's src/core,
which means it's not meant to be #included outside of Skia proper (it's not in
the public API, although it's just a typedef now anyway). Skipping backwards
also looks dubious, as Peter pointed out. It looks like the code is trying to
skip over the shader by sniffing the size written in the already-read int, which
looks fragile.

Anyway, this is only test code, but I think the correct thing here is to call
the base class constructor with the read buffer; something like:

TestDiscardableShader(SkReadBuffer& buffer) : SkShader(buffer) {
    CreateBitmap(gfx::Size(50, 50), "discardable", &bitmap_);
}

Could you try that and see if the test still passes?

(Or Vlad, if there was some reason this was necessary, could you enlighten us?)

[vmpstr, 2014-07-10 21:55:38]

On 2014/07/10 02:13:56, Stephen White wrote:
>
https://codereview.chromium.org/382673002/diff/1/skia/ext/pixel_ref_utils_uni...
> File skia/ext/pixel_ref_utils_unittest.cc (right):
> 
>
https://codereview.chromium.org/382673002/diff/1/skia/ext/pixel_ref_utils_uni...
> skia/ext/pixel_ref_utils_unittest.cc:39:
reader->skip(static_cast<size_t>(-4)); 
> //!!? Is this safe?
> On 2014/07/09 22:19:54, Peter Kasting wrote:
> > This needs review.  Static-casting a negative value to an unsigned one and
> then
> > actually using it like a negative value seems dangerous to me (means we have
> > code relying on integer overflow); it suggests the signature of skip()
> shouldn't
> > be taking an unsigned value if we want it to be able to move backwards.
> 
> +vmpstr +junov
> 
> Much of this code looks dubious. SkOrderedReadBuffer is in Skia's src/core,
> which means it's not meant to be #included outside of Skia proper (it's not in
> the public API, although it's just a typedef now anyway). Skipping backwards
> also looks dubious, as Peter pointed out. It looks like the code is trying to
> skip over the shader by sniffing the size written in the already-read int,
which
> looks fragile.
> 
> Anyway, this is only test code, but I think the correct thing here is to call
> the base class constructor with the read buffer; something like:
> 
> TestDiscardableShader(SkReadBuffer& buffer) : SkShader(buffer) {
>     CreateBitmap(gfx::Size(50, 50), "discardable", &bitmap_);
> }
> 
> Could you try that and see if the test still passes?
> 
> (Or Vlad, if there was some reason this was necessary, could you enlighten
us?)

Honestly, a lot has changed since this was put in and I didn't quite keep up
with what has changed. It looks like this constructor is used as a part of
SK_DECLARE_PUBLIC_FLATTENABLE_DESERIALIZATION_PROCS. Is it possible to remove
that declaration and the constructor?

If not, then I think senorblanco@'s suggestion would work.

[Peter Kasting, 2014-07-10 23:01:47]

New snap up.

On 2014/07/10 21:55:38, vmpstr wrote:
> On 2014/07/10 02:13:56, Stephen White wrote:
> > Anyway, this is only test code, but I think the correct thing here is to
call
> > the base class constructor with the read buffer; something like:
> > 
> > TestDiscardableShader(SkReadBuffer& buffer) : SkShader(buffer) {
> >     CreateBitmap(gfx::Size(50, 50), "discardable", &bitmap_);
> > }
> > 
> > Could you try that and see if the test still passes?

The test seems to pass locally for me with this change.  I've uploaded it.

> It looks like this constructor is used as a part of
> SK_DECLARE_PUBLIC_FLATTENABLE_DESERIALIZATION_PROCS. Is it possible to remove
> that declaration and the constructor?

I tried removing that declaration and the constructor entirely, and the file
failed to compile (basically, due to an abstract interface not being fully
concretely implemented).

[Stephen White, 2014-07-11 14:07:37]

skia/ext LGTM

[Peter Kasting, 2014-07-11 17:22:03]

Friendly ping for Darin

[Peter Kasting, 2014-07-14 22:18:07]

Ben, Darin seems to be unresponsive, could you review this?

[Peter Kasting, 2014-07-16 20:19:59]

Ping for either Ben or Darin -- you guys are the two top-level OWNERS, so I'd
appreciate a signoff from one of you

[Ben Goodger (Google), 2014-07-16 20:31:47]

lgtm

[Peter Kasting, 2014-07-16 20:32:53]

The CQ bit was checked by pkasting@chromium.org

[commit-bot: I haz the power, 2014-07-16 20:37:29]

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

[commit-bot: I haz the power, 2014-07-16 20:45:25]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)
  linux_chromium_gn_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...)
  win8_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win8_chromium_rel/bui...)

[commit-bot: I haz the power, 2014-07-16 20:51:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-07-16 20:51:19]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)
  linux_chromium_gn_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...)
  win8_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win8_chromium_rel/bui...)

[Peter Kasting, 2014-07-16 20:53:24]

The CQ bit was checked by pkasting@chromium.org

[commit-bot: I haz the power, 2014-07-16 20:55:15]

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

[commit-bot: I haz the power, 2014-07-17 00:34:12]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)

[commit-bot: I haz the power, 2014-07-17 01:07:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-07-17 01:07:55]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)

[Peter Kasting, 2014-07-17 01:09:59]

+rpaquay for device/bluetooth/bluetooth_socket_win.cc

[rpaquay, 2014-07-17 18:27:36]

lgtm

[Peter Kasting, 2014-07-17 18:27:51]

The CQ bit was checked by pkasting@chromium.org

[commit-bot: I haz the power, 2014-07-17 18:30:49]

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

[commit-bot: I haz the power, 2014-07-17 19:14:07]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/29893)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/34303)

[commit-bot: I haz the power, 2014-07-17 19:18:58]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-07-17 19:18:59]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/29921)

[Peter Kasting, 2014-07-17 21:47:49]

The CQ bit was checked by pkasting@chromium.org

[commit-bot: I haz the power, 2014-07-17 21:48:39]

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

[commit-bot: I haz the power, 2014-07-18 01:40:57]

Change committed as 283967