Type conversion fixes, gpu/ edition.

Type conversion fixes, gpu/ edition.

This is mostly to fix MSVC warnings about possible value truncation.

BUG=81439
TEST=none

Committed: https://crrev.com/45796dc81f2730136f3c0218cf611a339837b682
Cr-Commit-Position: refs/heads/master@{#300060}

[Peter Kasting, 2014-10-16 23:17:09]

pkasting@chromium.org changed reviewers:
+ sievers@chromium.org

[Peter Kasting, 2014-10-16 23:20:11]

https://codereview.chromium.org/660093004/diff/1/gpu/command_buffer/service/g...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/660093004/diff/1/gpu/command_buffer/service/g...
gpu/command_buffer/service/gles2_cmd_decoder.cc:7218: GLboolean normalized =
!!c.normalized;
This is a bit subtle.

c.normalized is a 32-bit value, while GLboolean is an 8-bit type (not a bool).

Therefore, an explicit or implicit cast directly to GLboolean is functionally
equivalent to something like:

  normalized = c.normalized & 0xff;

...meaning that, depending on the value of c.normalized, this could convert a
nonzero source value into a zero destination, or convert something into a
destination value other than 0 or 1.

I assumed this wasn't the intent of the code, and the real intent was more like:

  normalized = c.normalized != 0;

"!!" is simply a shorter way of writing that same expression.  (2 places)

[no sievers, 2014-10-16 23:52:08]

LGTM but feel free to fix the existing glitch in texture_manager.cc:1296.

https://codereview.chromium.org/660093004/diff/1/gpu/command_buffer/service/g...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/660093004/diff/1/gpu/command_buffer/service/g...
gpu/command_buffer/service/gles2_cmd_decoder.cc:7218: GLboolean normalized =
!!c.normalized;
On 2014/10/16 23:20:11, Peter Kasting wrote:
> This is a bit subtle.
> 
> c.normalized is a 32-bit value, while GLboolean is an 8-bit type (not a bool).
> 
> Therefore, an explicit or implicit cast directly to GLboolean is functionally
> equivalent to something like:
> 
>   normalized = c.normalized & 0xff;
> 
> ...meaning that, depending on the value of c.normalized, this could convert a
> nonzero source value into a zero destination, or convert something into a
> destination value other than 0 or 1.
> 
> I assumed this wasn't the intent of the code, and the real intent was more
like:
> 
>   normalized = c.normalized != 0;
> 
> "!!" is simply a shorter way of writing that same expression.  (2 places)

Yes this looks good since this gets set from gles2_cmd_helper_autogen.h where it
expands GLBoolean to 32bits. It's just that in shared memory we want every field
to be 32bit so we can use pragma pack(4).

https://codereview.chromium.org/660093004/diff/1/gpu/command_buffer/service/g...
gpu/command_buffer/service/gles2_cmd_decoder.cc:7460: GLboolean async =
!!c.async;
same here

https://codereview.chromium.org/660093004/diff/1/gpu/command_buffer/service/t...
File gpu/command_buffer/service/texture_manager.cc (left):

https://codereview.chromium.org/660093004/diff/1/gpu/command_buffer/service/t...
gpu/command_buffer/service/texture_manager.cc:1296: error_state, function_name,
param, "param");
I think this is a typo here. If the error is INVALID_ENUM, you want to log
|pname|. See error_state.cc SetGLErrorInvalidEnum() which converts tries to
convert this enum to a string.

[Peter Kasting, 2014-10-17 00:00:40]

https://codereview.chromium.org/660093004/diff/1/gpu/command_buffer/service/g...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/660093004/diff/1/gpu/command_buffer/service/g...
gpu/command_buffer/service/gles2_cmd_decoder.cc:7218: GLboolean normalized =
!!c.normalized;
On 2014/10/16 23:52:08, sievers wrote:
> On 2014/10/16 23:20:11, Peter Kasting wrote:
> > This is a bit subtle.
> > 
> > c.normalized is a 32-bit value, while GLboolean is an 8-bit type (not a
bool).
> > 
> > Therefore, an explicit or implicit cast directly to GLboolean is
functionally
> > equivalent to something like:
> > 
> >   normalized = c.normalized & 0xff;
> > 
> > ...meaning that, depending on the value of c.normalized, this could convert
a
> > nonzero source value into a zero destination, or convert something into a
> > destination value other than 0 or 1.
> > 
> > I assumed this wasn't the intent of the code, and the real intent was more
> like:
> > 
> >   normalized = c.normalized != 0;
> > 
> > "!!" is simply a shorter way of writing that same expression.  (2 places)
> 
> Yes this looks good since this gets set from gles2_cmd_helper_autogen.h where
it
> expands GLBoolean to 32bits. It's just that in shared memory we want every
field
> to be 32bit so we can use pragma pack(4).

I see.

I wonder if it might be better to use static_cast<GLboolean>, then, if the
original source value was a GLboolean.  That way, if for some insane reason
someone had a use for an original value > 1, it would be preserved correctly
here.

Which do you think is better?

https://codereview.chromium.org/660093004/diff/1/gpu/command_buffer/service/t...
File gpu/command_buffer/service/texture_manager.cc (left):

https://codereview.chromium.org/660093004/diff/1/gpu/command_buffer/service/t...
gpu/command_buffer/service/texture_manager.cc:1296: error_state, function_name,
param, "param");
On 2014/10/16 23:52:08, sievers wrote:
> I think this is a typo here. If the error is INVALID_ENUM, you want to log
> |pname|. See error_state.cc SetGLErrorInvalidEnum() which converts tries to
> convert this enum to a string.

So, like this?:

      ERRORSTATE_SET_GL_ERROR_INVALID_ENUM(
          error_state, function_name, pname, "pname");

[no sievers, 2014-10-17 00:06:24]

https://codereview.chromium.org/660093004/diff/1/gpu/command_buffer/service/g...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/660093004/diff/1/gpu/command_buffer/service/g...
gpu/command_buffer/service/gles2_cmd_decoder.cc:7218: GLboolean normalized =
!!c.normalized;
On 2014/10/17 00:00:40, Peter Kasting wrote:
> On 2014/10/16 23:52:08, sievers wrote:
> > On 2014/10/16 23:20:11, Peter Kasting wrote:
> > > This is a bit subtle.
> > > 
> > > c.normalized is a 32-bit value, while GLboolean is an 8-bit type (not a
> bool).
> > > 
> > > Therefore, an explicit or implicit cast directly to GLboolean is
> functionally
> > > equivalent to something like:
> > > 
> > >   normalized = c.normalized & 0xff;
> > > 
> > > ...meaning that, depending on the value of c.normalized, this could
convert
> a
> > > nonzero source value into a zero destination, or convert something into a
> > > destination value other than 0 or 1.
> > > 
> > > I assumed this wasn't the intent of the code, and the real intent was more
> > like:
> > > 
> > >   normalized = c.normalized != 0;
> > > 
> > > "!!" is simply a shorter way of writing that same expression.  (2 places)
> > 
> > Yes this looks good since this gets set from gles2_cmd_helper_autogen.h
where
> it
> > expands GLBoolean to 32bits. It's just that in shared memory we want every
> field
> > to be 32bit so we can use pragma pack(4).
> 
> I see.
> 
> I wonder if it might be better to use static_cast<GLboolean>, then, if the
> original source value was a GLboolean.  That way, if for some insane reason
> someone had a use for an original value > 1, it would be preserved correctly
> here.
> 
> Which do you think is better?

Yea maybe it's more correct to do the inverse (i.e. static_cast<GLBoolean>) of
the expansion we do in the client (implicit conversion to 32bit) here.

Either way it would be a client bug if that affects behavior :)

https://codereview.chromium.org/660093004/diff/1/gpu/command_buffer/service/t...
File gpu/command_buffer/service/texture_manager.cc (left):

https://codereview.chromium.org/660093004/diff/1/gpu/command_buffer/service/t...
gpu/command_buffer/service/texture_manager.cc:1296: error_state, function_name,
param, "param");
On 2014/10/17 00:00:40, Peter Kasting wrote:
> On 2014/10/16 23:52:08, sievers wrote:
> > I think this is a typo here. If the error is INVALID_ENUM, you want to log
> > |pname|. See error_state.cc SetGLErrorInvalidEnum() which converts tries to
> > convert this enum to a string.
> 
> So, like this?:
> 
>       ERRORSTATE_SET_GL_ERROR_INVALID_ENUM(
>           error_state, function_name, pname, "pname");

Yes.

[Peter Kasting, 2014-10-17 00:40:42]

OK, made both changes.  Landing.

[Peter Kasting, 2014-10-17 00:40:45]

The CQ bit was checked by pkasting@chromium.org

[commit-bot: I haz the power, 2014-10-17 00:41:16]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/660093004/20001

[commit-bot: I haz the power, 2014-10-17 02:42:36]

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

[commit-bot: I haz the power, 2014-10-17 02:42:37]

Exceeded time limit waiting for builds to trigger.

[Peter Kasting, 2014-10-17 04:15:46]

The CQ bit was checked by pkasting@chromium.org

[commit-bot: I haz the power, 2014-10-17 04:16:32]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/660093004/20001

[commit-bot: I haz the power, 2014-10-17 04:18:40]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2014-10-17 04:19:20]

Patchset 2 (id:??) landed as
https://crrev.com/45796dc81f2730136f3c0218cf611a339837b682
Cr-Commit-Position: refs/heads/master@{#300060}