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

skia/ext/pixel_ref_utils_unittest.cc

Index: skia/ext/pixel_ref_utils_unittest.cc
diff --git a/skia/ext/pixel_ref_utils_unittest.cc b/skia/ext/pixel_ref_utils_unittest.cc
index bddbe657934d7f5e5a5f6c01193ef148594d72eb..dfd0ddb081ca9e4eee299d8c3642a758e6e3699e 100644
--- a/skia/ext/pixel_ref_utils_unittest.cc
+++ b/skia/ext/pixel_ref_utils_unittest.cc
@@ -36,7 +36,7 @@ class TestDiscardableShader : public SkShader {
         static_cast<SkOrderedReadBuffer&>(flattenable_buffer);
     SkReader32* reader = buffer.getReader32();
 
-    reader->skip(-4);
+    reader->skip(static_cast<size_t>(-4));  //!!? Is this safe?

[Peter Kasting, 2014/07/09 22:19:54]

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.

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

+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?)

     uint32_t toSkip = reader->readU32();
     reader->skip(toSkip);