Address MSVC warnings about possible value truncation.

include/core/SkColorPriv.h

Index: include/core/SkColorPriv.h
diff --git a/include/core/SkColorPriv.h b/include/core/SkColorPriv.h
index c8d71a5a8c21c22c5cdb81caca94b2be44208a0b..540d999830959c68a170074931f7b74bcea23371 100644
--- a/include/core/SkColorPriv.h
+++ b/include/core/SkColorPriv.h
@@ -306,7 +306,8 @@ static inline void SkBlendRGB16(const uint16_t src[], uint16_t dst[],
     do {
         uint32_t src32 = SkExpand_rgb_16(*src++);
         uint32_t dst32 = SkExpand_rgb_16(*dst);
-        *dst++ = SkCompact_rgb_16(dst32 + ((src32 - dst32) * srcScale >> 5));
+        *dst++ = static_cast<uint16_t>(
+            SkCompact_rgb_16(dst32 + ((src32 - dst32) * srcScale >> 5)));

[Peter Kasting, 2014/08/28 21:13:54]

This function returns a U16CPU for speed, expecting callers to explicitly ignore
the upper bits if needed.  It looked to be called in several places, so I
elected not to change its signature.

[reed1, 2014/08/28 21:30:00]

SkToU16() might be even better, since it checks that we indeed fit in 16bits
(but only in debugging mode).

[Peter Kasting, 2014/08/28 21:58:23]

Wouldn't that potentially cause errors if the upper bits are dirty?  I thought
the point of U16CPU was to avoid forcing the compiler to waste time clearing the
upper bits in some cases.

I'm happy to do what you'd like here, I just want to make sure such a function
call is both correct and actually necessary.

[reed1, 2014/08/29 12:47:08]

SkToU16 just does a cast in release mode (as your code does). In debug, it
asserts that indeed the upper 16bits are 0. This is the promise of
SkCompact_rgb_16 (otherwise it may have a bug).

     } while (--count > 0);
 }
 
@@ -785,7 +786,7 @@ static inline SkPMColor16 SkPackARGB4444(unsigned a, unsigned r,
                          (g << SK_G4444_SHIFT) | (b << SK_B4444_SHIFT));
 }
 
-static inline U16CPU SkAlphaMulQ4(U16CPU c, unsigned scale) {
+static inline SkPMColor16 SkAlphaMulQ4(SkPMColor16 c, int scale) {

[Peter Kasting, 2014/08/28 21:13:54]

Seemingly only used in SkBlend4444To16() below, which uses these types as
args/expected return.

     SkASSERT(scale <= 16);
 
     const unsigned mask = 0xF0F;    //gMask_0F0F;
@@ -796,33 +797,21 @@ static inline U16CPU SkAlphaMulQ4(U16CPU c, unsigned scale) {
     return (rb & mask) | (ag & ~mask);
 #else
     c = (c & mask) | ((c & (mask << 4)) << 12);

[reed1, 2014/08/28 21:29:59]

If we're going to use a 16bit type in the parameter (c), then we should locally
have a different temporary for these two lines (where we assign into c.
Otherwise the compiler "may" decide to truncate down to 16bits on each
assignment.

[Peter Kasting, 2014/08/28 21:58:23]

Not just may, will, which would break the calculation entirely.  Oops.

Changed to use temps, but I probably picked crappy names for them.

-    c = c * scale >> 4;
+    c = static_cast<SkPMColor16>((c * scale) >> 4);
     return (c & mask) | ((c >> 12) & (mask << 4));
 #endif
 }
 
-/** Expand the SkPMColor16 color into a 32bit value that can be scaled all at
-    once by a value up to 16. Used in conjunction with SkCompact_4444.
-*/
-static inline uint32_t SkExpand_4444(U16CPU c) {
-    SkASSERT(c == (uint16_t)c);
-
-    const unsigned mask = 0xF0F;    //gMask_0F0F;
-    return (c & mask) | ((c & ~mask) << 12);
-}
-
-/** Compress an expanded value (from SkExpand_4444) back down to a SkPMColor16.
-    NOTE: this explicitly does not clean the top 16 bits (which may be garbage).
-    It does this for speed, since if it is being written directly to 16bits of
-    memory, the top 16bits will be ignored. Casting the result to uint16_t here
-    would add 2 more instructions, slow us down. It is up to the caller to
-    perform the cast if needed.
-*/
-static inline U16CPU SkCompact_4444(uint32_t c) {

[Peter Kasting, 2014/08/28 21:13:54]

Seemingly only used in SkBlend4444() below, which is going away.

[reed1, 2014/08/28 21:29:59]

Acknowledged.

-    const unsigned mask = 0xF0F;    //gMask_0F0F;
-    return (c & mask) | ((c >> 12) & ~mask);
-}
-
+/** Expand the SkPMColor16 color into a 32bit value that can be scaled all at
+    once by a value up to 16.
+*/
+static inline uint32_t SkExpand_4444(U16CPU c) {
+    SkASSERT(c == (uint16_t)c);
+
+    const unsigned mask = 0xF0F;    //gMask_0F0F;
+    return (c & mask) | ((c & ~mask) << 12);
+}
+
 static inline uint16_t SkSrcOver4444To16(SkPMColor16 s, uint16_t d) {
     unsigned sa = SkGetPackedA4444(s);
     unsigned sr = SkR4444ToR565(SkGetPackedR4444(s));
@@ -854,22 +843,6 @@ static inline uint16_t SkBlend4444To16(SkPMColor16 src, uint16_t dst, int scale1
     return SkSrcOver4444To16(SkAlphaMulQ4(src, scale16), dst);
 }
 
-static inline uint16_t SkBlend4444(SkPMColor16 src, SkPMColor16 dst, int scale16) {

[Peter Kasting, 2014/08/28 21:13:54]

Seemingly unused.

[reed1, 2014/08/28 21:30:00]

Acknowledged.

-    SkASSERT((unsigned)scale16 <= 16);
-
-    uint32_t src32 = SkExpand_4444(src) * scale16;
-    // the scaled srcAlpha is the bottom byte
-#ifdef SK_DEBUG
-    {
-        unsigned srcA = SkGetPackedA4444(src) * scale16;
-        SkASSERT(srcA == (src32 & 0xFF));
-    }
-#endif
-    unsigned dstScale = SkAlpha255To256(255 - (src32 & 0xFF)) >> 4;
-    uint32_t dst32 = SkExpand_4444(dst) * dstScale;
-    return SkCompact_4444((src32 + dst32) >> 4);
-}
-
 static inline SkPMColor SkPixel4444ToPixel32(U16CPU c) {
     uint32_t d = (SkGetPackedA4444(c) << SK_A32_SHIFT) |
                  (SkGetPackedR4444(c) << SK_R32_SHIFT) |