Address MSVC warnings about possible value truncation.

Address MSVC warnings about possible value truncation.  In the process removes some apparently unused code.

BUG=81439
TEST=none

Committed: https://skia.googlesource.com/skia/+/280c2c6fbbf953d83710d2229280075585a1ab59

[Peter Kasting, 2014-08-28 21:13:39]

pkasting@chromium.org changed reviewers:
+ reed@google.com

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

https://codereview.chromium.org/517763002/diff/1/include/core/SkColorPriv.h
File include/core/SkColorPriv.h (left):

https://codereview.chromium.org/517763002/diff/1/include/core/SkColorPriv.h#o...
include/core/SkColorPriv.h:821: static inline U16CPU SkCompact_4444(uint32_t c)
{
Seemingly only used in SkBlend4444() below, which is going away.

https://codereview.chromium.org/517763002/diff/1/include/core/SkColorPriv.h#o...
include/core/SkColorPriv.h:857: static inline uint16_t SkBlend4444(SkPMColor16
src, SkPMColor16 dst, int scale16) {
Seemingly unused.

https://codereview.chromium.org/517763002/diff/1/include/core/SkColorPriv.h
File include/core/SkColorPriv.h (right):

https://codereview.chromium.org/517763002/diff/1/include/core/SkColorPriv.h#n...
include/core/SkColorPriv.h:310: SkCompact_rgb_16(dst32 + ((src32 - dst32) *
srcScale >> 5)));
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.

https://codereview.chromium.org/517763002/diff/1/include/core/SkColorPriv.h#n...
include/core/SkColorPriv.h:789: static inline SkPMColor16
SkAlphaMulQ4(SkPMColor16 c, int scale) {
Seemingly only used in SkBlend4444To16() below, which uses these types as
args/expected return.

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

https://codereview.chromium.org/517763002/diff/1/include/core/SkColorPriv.h
File include/core/SkColorPriv.h (left):

https://codereview.chromium.org/517763002/diff/1/include/core/SkColorPriv.h#o...
include/core/SkColorPriv.h:821: static inline U16CPU SkCompact_4444(uint32_t c)
{
On 2014/08/28 21:13:54, Peter Kasting wrote:
> Seemingly only used in SkBlend4444() below, which is going away.

Acknowledged.

https://codereview.chromium.org/517763002/diff/1/include/core/SkColorPriv.h#o...
include/core/SkColorPriv.h:857: static inline uint16_t SkBlend4444(SkPMColor16
src, SkPMColor16 dst, int scale16) {
On 2014/08/28 21:13:54, Peter Kasting wrote:
> Seemingly unused.

Acknowledged.

https://codereview.chromium.org/517763002/diff/1/include/core/SkColorPriv.h
File include/core/SkColorPriv.h (right):

https://codereview.chromium.org/517763002/diff/1/include/core/SkColorPriv.h#n...
include/core/SkColorPriv.h:310: SkCompact_rgb_16(dst32 + ((src32 - dst32) *
srcScale >> 5)));
On 2014/08/28 21:13:54, Peter Kasting wrote:
> 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.

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

https://codereview.chromium.org/517763002/diff/1/include/core/SkColorPriv.h#n...
include/core/SkColorPriv.h:799: c = (c & mask) | ((c & (mask << 4)) << 12);
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]

Updated, PTAL.

https://codereview.chromium.org/517763002/diff/1/include/core/SkColorPriv.h
File include/core/SkColorPriv.h (right):

https://codereview.chromium.org/517763002/diff/1/include/core/SkColorPriv.h#n...
include/core/SkColorPriv.h:310: SkCompact_rgb_16(dst32 + ((src32 - dst32) *
srcScale >> 5)));
On 2014/08/28 21:30:00, reed1 wrote:
> On 2014/08/28 21:13:54, Peter Kasting wrote:
> > 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.
> 
> SkToU16() might be even better, since it checks that we indeed fit in 16bits
> (but only in debugging mode).

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.

https://codereview.chromium.org/517763002/diff/1/include/core/SkColorPriv.h#n...
include/core/SkColorPriv.h:799: c = (c & mask) | ((c & (mask << 4)) << 12);
On 2014/08/28 21:29:59, reed1 wrote:
> 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.

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

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

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

https://codereview.chromium.org/517763002/diff/1/include/core/SkColorPriv.h
File include/core/SkColorPriv.h (right):

https://codereview.chromium.org/517763002/diff/1/include/core/SkColorPriv.h#n...
include/core/SkColorPriv.h:310: SkCompact_rgb_16(dst32 + ((src32 - dst32) *
srcScale >> 5)));
On 2014/08/28 21:58:23, Peter Kasting wrote:
> On 2014/08/28 21:30:00, reed1 wrote:
> > On 2014/08/28 21:13:54, Peter Kasting wrote:
> > > 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.
> > 
> > SkToU16() might be even better, since it checks that we indeed fit in 16bits
> > (but only in debugging mode).
> 
> 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.

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).

[reed1, 2014-08-29 12:48:42]

https://codereview.chromium.org/517763002/diff/20001/include/core/SkColorPriv.h
File include/core/SkColorPriv.h (right):

https://codereview.chromium.org/517763002/diff/20001/include/core/SkColorPriv...
include/core/SkColorPriv.h:801: return (scaled_c & mask) | ((scaled_c >> 12) &
(mask << 4));
since you changed the return type to be explicitly 16bits, will this return
expression generate a warning? Will it generate extraneous shifts/masks to
"ensure" that we're only returning 16bits?

[Peter Kasting, 2014-08-29 20:50:35]

https://codereview.chromium.org/517763002/diff/20001/include/core/SkColorPriv.h
File include/core/SkColorPriv.h (right):

https://codereview.chromium.org/517763002/diff/20001/include/core/SkColorPriv...
include/core/SkColorPriv.h:801: return (scaled_c & mask) | ((scaled_c >> 12) &
(mask << 4));
On 2014/08/29 12:48:42, reed1 wrote:
> since you changed the return type to be explicitly 16bits, will this return
> expression generate a warning?

No.  MSVC is smart enough to realize that the result of this expression will
always fit in 16 bits.

> Will it generate extraneous shifts/masks to
> "ensure" that we're only returning 16bits?

I don't know, but if it does, then the existing code must be generating those in
order to perform the U16CPU -> SkPMColor16 truncation on the caller side
(remember that this function is only used in one caller below, which is storing
the result in an SkPMColor16), so at worst, we're moving those shifts/masks from
the caller side to here.

But I suspect that, since the compile can figure out that the value here won't
be truncated, it's not generating any masks either.  So at best, we're actually
removing any existing caller-side shift/mask code (since the caller has less
visibility into the construction of the returned value), thus improving perf.

[reed1, 2014-09-02 14:10:07]

lgtm

https://codereview.chromium.org/517763002/diff/20001/include/core/SkColorPriv.h
File include/core/SkColorPriv.h (right):

https://codereview.chromium.org/517763002/diff/20001/include/core/SkColorPriv...
include/core/SkColorPriv.h:801: return (scaled_c & mask) | ((scaled_c >> 12) &
(mask << 4));
On 2014/08/29 20:50:35, Peter Kasting wrote:
> On 2014/08/29 12:48:42, reed1 wrote:
> > since you changed the return type to be explicitly 16bits, will this return
> > expression generate a warning?
> 
> No.  <span id="abbrex-spnAbbr_14193" class="abbrex-Abbr abbrex-Abbr_MSVC
abbrex-AbbrUnknown abbrex-AbbrUnknownVisible">MSVC</span> is smart enough to
realize that the result of this expression will
> always fit in 16 bits.
> 
> > Will it generate extraneous shifts/masks to
> > "ensure" that we're only returning 16bits?
> 
> I don't know, but if it does, then the existing code must be generating those
in
> order to perform the U16CPU -> SkPMColor16 truncation on the caller side
> (remember that this function is only used in one caller below, which is
storing
> the result in an SkPMColor16), so at worst, we're moving those shifts/masks
from
> the caller side to here.
> 
> But I suspect that, since the compile can figure out that the value here won't
> be truncated, it's not generating any masks either.  So at best, we're
actually
> removing any existing caller-side shift/mask code (since the caller has less
> visibility into the construction of the returned value), thus improving perf.

Ack - you're right. The caller is SkSrcOver4444To16() which takes SkPMColor16.
That is a shame, as it could also have taken U16CPU to be safe.

I'm fine with this either way for now.

[Peter Kasting, 2014-09-02 19:54:40]

The CQ bit was checked by pkasting@chromium.org

[commit-bot: I haz the power, 2014-09-02 19:55:08]

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

[Peter Kasting, 2014-09-02 20:05:51]

Mike, the patch looks to be failing due to the assertion from the added
SkToU16() call in SkBlendRGB16().  Basically, what I was concerned about seems
to be the case: SkCompact_rgb_16() can leave the upper 16 bits dirty.

Looking at the code for that function, it makes no effort to zero the top 16
bits of the 32-bit input argument, so I'd expect it to potentially leave dirty
bits; and the comments explicitly say that  the top bits can be dirty and it's
up to callers to ignore them.

So I think I need to use an explicit cast instead of SkToU16().  This shouldn't
result in any additional generated code over the checked-in version of the
function.

[Peter Kasting, 2014-09-02 20:06:04]

The CQ bit was unchecked by pkasting@chromium.org

[reed1, 2014-09-02 20:16:34]

Yea, I see that it, and the other functions that call it within SkColorPriv.h
all document that they return dirty bits in the upper half (for speed).

I'm wondering why you need the SkToU16() call at all now, given the many other
places where we directly assign SkCompact_rgb_16() to a uint16_t pointer
(apparently w/ no warning). See src/core/SkBitmapProcState_procs.h:[258,275,295]

All of that said, I agree that if you do need a cast when assigning to a 16bit
ptr (e.g. *dst++ = SkCompact_rgb_16) then a straight cast is correct whereas
SkToU16 is not).

[Peter Kasting, 2014-09-02 20:25:06]

On 2014/09/02 20:16:34, reed1 wrote:
> Yea, I see that it, and the other functions that call it within SkColorPriv.h
> all document that they return dirty bits in the upper half (for speed).
> 
> I'm wondering why you need the SkToU16() call at all now, given the many other
> places where we directly assign SkCompact_rgb_16() to a uint16_t pointer
> (apparently w/ no warning). See
src/core/SkBitmapProcState_procs.h:[258,275,295]

It's possible those places aren't actually used with a 16-bit destination type,
at least on Windows.  Because these cases occur in macros, the compiler will
only warn if some caller code uses the particular macro expansions in question.

Another possibility is that these will trigger a warning too, and I simply
haven't gotten to that warning yet.  I haven't made it all the way through the
full warning list in Chromium, since it's several thousand files; I'm trying to
do the work in big chunks.  Usually that means I get all warnings for a
particular module at once, but again, since these cases involve macros in
headers, it could be that I won't see the warnings until I compile some random
Chromium UI code that #includes this.

I think for now I'd like to make the location here do a straight cast.  We can
revisit this in the future if necessary.

[Peter Kasting, 2014-09-02 20:32:09]

The CQ bit was checked by pkasting@chromium.org

[commit-bot: I haz the power, 2014-09-02 20:33:08]

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

[reed1, 2014-09-02 20:37:02]

On 2014/09/02 20:25:06, Peter Kasting wrote:
> On 2014/09/02 20:16:34, reed1 wrote:
> > Yea, I see that it, and the other functions that call it within
SkColorPriv.h
> > all document that they return dirty bits in the upper half (for speed).
> > 
> > I'm wondering why you need the SkToU16() call at all now, given the many
other
> > places where we directly assign SkCompact_rgb_16() to a uint16_t pointer
> > (apparently w/ no warning). See
> src/core/SkBitmapProcState_procs.h:[258,275,295]
> 
> It's possible those places aren't actually used with a 16-bit destination
type,
> at least on Windows.  Because these cases occur in macros, the compiler will
> only warn if some caller code uses the particular macro expansions in
question.
> 
> Another possibility is that these will trigger a warning too, and I simply
> haven't gotten to that warning yet.  I haven't made it all the way through the
> full warning list in Chromium, since it's several thousand files; I'm trying
to
> do the work in big chunks.  Usually that means I get all warnings for a
> particular module at once, but again, since these cases involve macros in
> headers, it could be that I won't see the warnings until I compile some random
> Chromium UI code that #includes this.
> 
> I think for now I'd like to make the location here do a straight cast.  We can
> revisit this in the future if necessary.

sgtm

[commit-bot: I haz the power, 2014-09-02 20:40:55]

Committed patchset #3 (id:40001) as 280c2c6fbbf953d83710d2229280075585a1ab59