gfx: Use unsigned strides for gpu memory buffers.

gfx: Use unsigned strides for gpu memory buffers.

BUG=none
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

[sadrul, 2016-08-31 18:57:32]

Description was changed from

==========
gfx: Use unsigned strides for gpu memory buffers.

BUG=none
==========

to

==========
gfx: Use unsigned strides for gpu memory buffers.

BUG=none
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[sadrul, 2016-08-31 18:57:32]

Description was changed from

==========
gfx: Use unsigned strides for gpu memory buffers.

BUG=none
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
gfx: Use unsigned strides for gpu memory buffers.

BUG=none
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[sadrul, 2016-08-31 18:58:08]

The CQ bit was checked by sadrul@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-31 18:58:28]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-31 19:13:06]

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

[commit-bot: I haz the power, 2016-08-31 19:13:07]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[sadrul, 2016-08-31 19:26:56]

The CQ bit was checked by sadrul@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-31 19:27:31]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[sadrul, 2016-08-31 19:28:39]

sadrul@chromium.org changed reviewers:
+ reveman@chromium.org, tsepez@chromium.org

[sadrul, 2016-08-31 19:28:40]

This came up during review of another CL:
https://codereview.chromium.org/2298003002/diff/140001/ui/gfx/mojo/buffer_typ...
PTAL. Thanks!

[spang, 2016-08-31 19:37:39]

On 2016/08/31 19:28:40, sadrul wrote:
> This came up during review of another CL:
>
https://codereview.chromium.org/2298003002/diff/140001/ui/gfx/mojo/buffer_typ...
> PTAL. Thanks!

stride is an arithmetic value (not a bitmask), and should be a signed integer.

"Avoid unsigned types" is a rule in the style guide:

https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#...

[sadrul, 2016-08-31 19:41:31]

sadrul@chromium.org changed reviewers:
+ rjkroege@chromium.org

[sadrul, 2016-08-31 19:41:31]

+rjkroege@

On 2016/08/31 19:37:39, spang wrote:
> On 2016/08/31 19:28:40, sadrul wrote:
> > This came up during review of another CL:
> >
>
https://codereview.chromium.org/2298003002/diff/140001/ui/gfx/mojo/buffer_typ...
> > PTAL. Thanks!
> 
> stride is an arithmetic value (not a bitmask), and should be a signed integer.
> 
> "Avoid unsigned types" is a rule in the style guide:
> 
>
https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#...

Rob also mentioned that signed strides may actually be used for some things
(like 'upside down scanout')? So perhaps we do not actually want to make this
unsigned?

[commit-bot: I haz the power, 2016-08-31 19:43:26]

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

[commit-bot: I haz the power, 2016-08-31 19:43:27]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[rjkroege, 2016-08-31 19:57:38]

On 2016/08/31 19:41:31, sadrul wrote:
> +rjkroege@
> 
> On 2016/08/31 19:37:39, spang wrote:
> > On 2016/08/31 19:28:40, sadrul wrote:
> > > This came up during review of another CL:
> > >
> >
>
https://codereview.chromium.org/2298003002/diff/140001/ui/gfx/mojo/buffer_typ...
> > > PTAL. Thanks!
> > 
> > stride is an arithmetic value (not a bitmask), and should be a signed
integer.
> > 
> > "Avoid unsigned types" is a rule in the style guide:
> > 
> >
>
https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#...
> 
> Rob also mentioned that signed strides may actually be used for some things
> (like 'upside down scanout')? So perhaps we do not actually want to make this
> unsigned?

we've at least talked about needing it so probably not lgtm. And the style guide
thing.

[Tom Sepez, 2016-08-31 20:10:02]

> > Rob also mentioned that signed strides may actually be used for some things
> > (like 'upside down scanout')? So perhaps we do not actually want to make
this
> > unsigned?
> 
> we've at least talked about needing it so probably not lgtm. And the style
guide
> thing.

We've got a situation where the style guide is conflicting with writing safe
code, particularly as compilers optimize aggressively around undefined behaviour
and remove what you thought were your sanity checks. But this probably isn't the
place to discuss it.

[spang, 2016-08-31 20:17:34]

On 2016/08/31 20:10:02, Tom Sepez wrote:
> > > Rob also mentioned that signed strides may actually be used for some
things
> > > (like 'upside down scanout')? So perhaps we do not actually want to make
> this
> > > unsigned?
> > 
> > we've at least talked about needing it so probably not lgtm. And the style
> guide
> > thing.
> 
> We've got a situation where the style guide is conflicting with writing safe
> code, particularly as compilers optimize aggressively around undefined
behaviour
> and remove what you thought were your sanity checks. But this probably isn't
the
> place to discuss it.
 
Yes, please take it up with the style arbiters rather than asking people to
ignore the style rules.

[Tom Sepez, 2016-08-31 20:21:26]

> Yes, please take it up with the style arbiters rather than asking people to
> ignore the style rules.
Thanks.

[Tom Sepez, 2016-08-31 20:25:34]

On 2016/08/31 20:21:26, Tom Sepez wrote:
> > Yes, please take it up with the style arbiters rather than asking people to
> > ignore the style rules.
> Thanks.

Turms out this is already taken care of, as the link that rjkroege sent is to
the corporate style guide, not the chromium one, which supersedes it. 
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...

[Tom Sepez, 2016-08-31 20:26:19]

Now, having said that, keeping the value signed for some future unimplemented
feature at the risk of having safer code seems dubious.

[sadrul, 2016-08-31 20:40:58]

The CQ bit was checked by sadrul@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-31 20:41:40]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[spang, 2016-08-31 20:41:51]

On 2016/08/31 20:26:19, Tom Sepez wrote:
> Now, having said that, keeping the value signed for some future unimplemented
> feature at the risk of having safer code seems dubious.

Ouch, that's confusing.

That says to use "size_t" though and still recommends against uintN_t et. al.

[Tom Sepez, 2016-08-31 20:44:47]

On 2016/08/31 20:41:51, spang wrote:
> On 2016/08/31 20:26:19, Tom Sepez wrote:
> > Now, having said that, keeping the value signed for some future
unimplemented
> > feature at the risk of having safer code seems dubious.
> 
> Ouch, that's confusing.
> 
> That says to use "size_t" though and still recommends against uintN_t et. al.

The key sentences are

. The signed types are incorrect and unsafe for these purposes (e.g. integer
overflow behavior for signed types is undefined in the C and C++ standards,
while the behavior is defined for unsigned types.) 

. When passing values across network or process boundaries, use explicitly-sized
types for safety.

the consequence is that we prefer size_t unless its going across a boundary, in
which case uintN_t is required.

[reveman, 2016-08-31 21:23:44]

We've intentionally kept stride signed so that GMBs can support y-flipped
scanout if needed in the future.

[Tom Sepez, 2016-08-31 21:42:05]

On 2016/08/31 21:23:44, reveman wrote:
> We've intentionally kept stride signed so that GMBs can support y-flipped
> scanout if needed in the future.
Ok.

[commit-bot: I haz the power, 2016-08-31 22:58:53]

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

[commit-bot: I haz the power, 2016-08-31 22:58:54]

Dry run: This issue passed the CQ dry run.

[danakj, 2016-09-01 21:33:45]

On Wed, Aug 31, 2016 at 1:44 PM, <tsepez@chromium.org> wrote:

> On 2016/08/31 20:41:51, spang wrote:
> > On 2016/08/31 20:26:19, Tom Sepez wrote:
> > > Now, having said that, keeping the value signed for some future
> unimplemented
> > > feature at the risk of having safer code seems dubious.
> >
> > Ouch, that's confusing.
> >
> > That says to use "size_t" though and still recommends against uintN_t
> et. al.
>
> The key sentences are
>
> . The signed types are incorrect and unsafe for these purposes (e.g.
> integer
> overflow behavior for signed types is undefined in the C and C++ standards,
> while the behavior is defined for unsigned types.)
>
> . When passing values across network or process boundaries, use
> explicitly-sized
> types for safety.
>
> the consequence is that we prefer size_t unless its going across a
> boundary, in
> which case uintN_t is required.
>

There was a good thread about this on cxx@chromium.org recently, with links
to internal g3 style discussions.
https://groups.google.com/a/chromium.org/d/topic/cxx/IyEs-62ml3I/discussion


>
>
> https://codereview.chromium.org/2298353002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[sadrul, 2016-09-02 01:50:45]

Closing this issue since we do want signed strides.