cc: Audit and static_assert the types in PaintOpBuffer can be moved.

cc: Audit and static_assert the types in PaintOpBuffer can be moved.

This verifies that we can move the types stored in PaintOpBuffer to
a new memory address without calling a move constructor or destructor,
by doing memcpy() and free()ing the old memory.

std::is_trivially_copyable() types satisfy this since they have a
trivial move constructor, but it requires a lot of other things we do
not require since we don't keep around both copies.

std::is_trivially_move_constructible() should satisfy this also, as we
are conceptually moving everything, but this is not available to us on
all platforms right now. I have left a TODO to this effect. It actually
provides more than we need as well, as it has built in the assumption
that we will call the destructor on the source object. sk_sp<> is an
example that does not satisfy this condition since it nulls out the
moved-from object. Since we don't ever use/destroy the moved-from
object this is not important for us, and sk_sp functions correctly when
we memcpy() and free() it, essentially moving its memory address.

Our requirements are that the object does not need to modify itself
when changing its address. In practice this generally means that it owns
no pointers to itself directly or indirectly. A bad example is that
std::vector can have a pointer back to the vector from its internal
buffer.

As of this patch no types in PaintOpBuffer are problematic. But now all
non-primitive types in PaintOps go through SafePaintOpData which does
the following:
- Adds static_asserts for is_trivially_copyable where possible.
- Has explanations on other types that we hope will be maintained /o\.

R=enne@chromium.org
BUG=671433
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

[danakj, 2017-05-19 20:30:27]

Description was changed from

==========
cc: Audit and static_assert the types in PaintOpBuffer can be moved.

This verifies that we can move the types stored in PaintOpBuffer to
a new memory address without calling a move constructor or destructor,
by doing memcpy() and free()ing the old memory.

std::is_trivially_copyable() types satisfy this since they have a
trivial move constructor, but it requires a lot of other things we do
not require since we don't keep around both copies.

std::is_trivially_move_constructible() should satisfy this also, as we
are conceptually moving everything, but this is not available to us on
all platforms right now. I have left a TODO to this effect. It actually
provides more than we need as well, as it has built in the assumption
that we will call the destructor on the source object. sk_sp<> is an
example that does not satisfy this condition since it nulls out the
moved-from object. Since we don't ever use/destroy the moved-from
object this is not important for us, and sk_sp functions correctly when
we memcpy() and free() it, essentially moving its memory address.

Our requirements are that the object does not need to modify itself
when changing its address. In practice this generally means that it owns
no pointers to itself directly or indirectly. A bad example is that
std::vector can have a pointer back to the vector from its internal
buffer.

As of this patch no types in PaintOpBuffer are problematic, but this
adds static_asserts for is_trivially_copyable where possible, comments
on known trivial types (such as bool), and explanations on other types
that we hope will be maintained /o\.

R=enne@chromium.org
BUG=671433
==========

to

==========
cc: Audit and static_assert the types in PaintOpBuffer can be moved.

This verifies that we can move the types stored in PaintOpBuffer to
a new memory address without calling a move constructor or destructor,
by doing memcpy() and free()ing the old memory.

std::is_trivially_copyable() types satisfy this since they have a
trivial move constructor, but it requires a lot of other things we do
not require since we don't keep around both copies.

std::is_trivially_move_constructible() should satisfy this also, as we
are conceptually moving everything, but this is not available to us on
all platforms right now. I have left a TODO to this effect. It actually
provides more than we need as well, as it has built in the assumption
that we will call the destructor on the source object. sk_sp<> is an
example that does not satisfy this condition since it nulls out the
moved-from object. Since we don't ever use/destroy the moved-from
object this is not important for us, and sk_sp functions correctly when
we memcpy() and free() it, essentially moving its memory address.

Our requirements are that the object does not need to modify itself
when changing its address. In practice this generally means that it owns
no pointers to itself directly or indirectly. A bad example is that
std::vector can have a pointer back to the vector from its internal
buffer.

As of this patch no types in PaintOpBuffer are problematic, but this
adds static_asserts for is_trivially_copyable where possible, comments
on known trivial types (such as bool), and explanations on other types
that we hope will be maintained /o\.

R=enne@chromium.org
BUG=671433
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[danakj, 2017-05-19 20:30:52]

Description was changed from

==========
cc: Audit and static_assert the types in PaintOpBuffer can be moved.

This verifies that we can move the types stored in PaintOpBuffer to
a new memory address without calling a move constructor or destructor,
by doing memcpy() and free()ing the old memory.

std::is_trivially_copyable() types satisfy this since they have a
trivial move constructor, but it requires a lot of other things we do
not require since we don't keep around both copies.

std::is_trivially_move_constructible() should satisfy this also, as we
are conceptually moving everything, but this is not available to us on
all platforms right now. I have left a TODO to this effect. It actually
provides more than we need as well, as it has built in the assumption
that we will call the destructor on the source object. sk_sp<> is an
example that does not satisfy this condition since it nulls out the
moved-from object. Since we don't ever use/destroy the moved-from
object this is not important for us, and sk_sp functions correctly when
we memcpy() and free() it, essentially moving its memory address.

Our requirements are that the object does not need to modify itself
when changing its address. In practice this generally means that it owns
no pointers to itself directly or indirectly. A bad example is that
std::vector can have a pointer back to the vector from its internal
buffer.

As of this patch no types in PaintOpBuffer are problematic, but this
adds static_asserts for is_trivially_copyable where possible, comments
on known trivial types (such as bool), and explanations on other types
that we hope will be maintained /o\.

R=enne@chromium.org
BUG=671433
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
cc: Audit and static_assert the types in PaintOpBuffer can be moved.

This verifies that we can move the types stored in PaintOpBuffer to
a new memory address without calling a move constructor or destructor,
by doing memcpy() and free()ing the old memory.

std::is_trivially_copyable() types satisfy this since they have a
trivial move constructor, but it requires a lot of other things we do
not require since we don't keep around both copies.

std::is_trivially_move_constructible() should satisfy this also, as we
are conceptually moving everything, but this is not available to us on
all platforms right now. I have left a TODO to this effect. It actually
provides more than we need as well, as it has built in the assumption
that we will call the destructor on the source object. sk_sp<> is an
example that does not satisfy this condition since it nulls out the
moved-from object. Since we don't ever use/destroy the moved-from
object this is not important for us, and sk_sp functions correctly when
we memcpy() and free() it, essentially moving its memory address.

Our requirements are that the object does not need to modify itself
when changing its address. In practice this generally means that it owns
no pointers to itself directly or indirectly. A bad example is that
std::vector can have a pointer back to the vector from its internal
buffer.

As of this patch no types in PaintOpBuffer are problematic, but this
adds static_asserts for is_trivially_copyable where possible, comments
on known trivial types (such as bool), and explanations on other types
that we hope will be maintained /o\.

R=enne@chromium.org
BUG=671433
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[danakj, 2017-05-19 20:32:55]

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

[commit-bot: I haz the power, 2017-05-19 20:33:45]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[danakj, 2017-05-19 20:34:16]

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

[commit-bot: I haz the power, 2017-05-19 20:34:30]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[danakj, 2017-05-19 20:35:16]

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

[commit-bot: I haz the power, 2017-05-19 20:35:27]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[danakj, 2017-05-19 20:35:35]

https://codereview.chromium.org/2899483002/diff/40001/cc/paint/paint_image.h
File cc/paint/paint_image.h (right):

https://codereview.chromium.org/2899483002/diff/40001/cc/paint/paint_image.h#...
cc/paint/paint_image.h:58: // Because PaintImage is stored in PaintOpBuffer it
must be able to be moved
I iterated on this comment a couple times to get something accurate and
descriptive, should be good now.

[commit-bot: I haz the power, 2017-05-19 22:36:54]

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

[commit-bot: I haz the power, 2017-05-19 22:36:55]

Dry run: This issue passed the CQ dry run.

[enne (OOO), 2017-05-22 17:19:45]

I...have a lot of mixed feelings about this patch.  I feel like the comments and
static asserts on everything does not merit enough protection for the decrease
in readability.

What about a whitelist of acceptable types at the top, with one static_assert
per type (that we can static assert on, and comments for the rest).  And one on
the array template class.  In general, I think that new member types are going
to be added rarely here and it's hard for me to imagine anything other than
std::vector that we're going to store in a POB that's going to not be safe.  It
just feels a bit like overkill.

If you wanted to make sure the whitelist was obeyed, I think I'd prefer a
presubmit of some sort over comments.  This removes the noise, makes the
checking automatic, and moves the checking out of the header itself.  wdyt?

(enum classes aren't trivially_copyable?!)

[danakj, 2017-05-23 17:04:58]

On Mon, May 22, 2017 at 1:19 PM, enne@chromium.org via
codereview.chromium.org <reply@chromiumcodereview-hr.appspotmail.com> wrote:

> I...have a lot of mixed feelings about this patch. I feel like the
> comments and
> static asserts on everything does not merit enough protection for the
> decrease
> in readability.
>
> What about a whitelist of acceptable types at the top, with one
> static_assert
> per type (that we can static assert on, and comments for the rest). And
> one on
> the array template class. In general, I think that new member types are
> going
> to be added rarely here and it's hard for me to imagine anything other than
> std::vector that we're going to store in a POB that's going to not be
> safe. It
> just feels a bit like overkill.
>

I could move it all to the top. The downside is that it's not as obvious
when someone adds a new field that it is or isn't in the whitelist and
reviewers are also less likely to remember to ask. I proposed this way to
take maximum advantage of cargo-culting.

I could put the static_asserts at the top, and refer to them from comments
instead. wdyt?


> If you wanted to make sure the whitelist was obeyed, I think I'd prefer a
> presubmit of some sort over comments.
>

I don't know how I would do this in a presubmit, as it is specific to
certain types (PaintOp subtypes), other than some elaborate multiline c++
parsing which I don't want to write.


> This removes the noise, makes the
> checking automatic, and moves the checking out of the header itself. wdyt?
>
> (enum classes aren't trivially_copyable?!)
>

Oh that's fair, they should be. I was thinking the types are in skia mostly
and out of our control but I could assert that. Not sure what I was
thinking.


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

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

[enne (OOO), 2017-05-23 18:15:25]

On 2017/05/23 at 17:04:58, danakj wrote:

> I could move it all to the top. The downside is that it's not as obvious
> when someone adds a new field that it is or isn't in the whitelist and
> reviewers are also less likely to remember to ask. I proposed this way to
> take maximum advantage of cargo-culting.
> 
> I could put the static_asserts at the top, and refer to them from comments
> instead. wdyt?

I understand why you did it.  I'm just not sure I buy the value of comments of
"// bool is a trivial type." everywhere over just trusting reviewers to be smart
here.
 
> > If you wanted to make sure the whitelist was obeyed, I think I'd prefer a
> > presubmit of some sort over comments.
> 
> I don't know how I would do this in a presubmit, as it is specific to
> certain types (PaintOp subtypes), other than some elaborate multiline c++
> parsing which I don't want to write.

Yeah.  I figured most of these PaintOps had a pretty regular structure (all
members separated by one line of whitespace before a "^};").  I know nobody
wants to parse C++ in Python, but it was a thought for automation since this was
all pretty regular.

My other thoughts were like declare all the members with some sort of
"decltype(IsSafeType<SkRect>::value) rect" template nonsense, but that's
suuuuuper wordy too.

[danakj, 2017-05-23 18:22:22]

On Tue, May 23, 2017 at 2:15 PM, enne@chromium.org via
codereview.chromium.org <reply@chromiumcodereview-hr.appspotmail.com> wrote:

> On 2017/05/23 at 17:04:58, danakj wrote:
>
> > I could move it all to the top. The downside is that it's not as obvious
> > when someone adds a new field that it is or isn't in the whitelist and
> > reviewers are also less likely to remember to ask. I proposed this way to
> > take maximum advantage of cargo-culting.
> >
> > I could put the static_asserts at the top, and refer to them from
> comments
> > instead. wdyt?
>
> I understand why you did it. I'm just not sure I buy the value of comments
> of
> "// bool is a trivial type." everywhere over just trusting reviewers to be
> smart
> here.
>

Yeah, I'm less worried about reviewers being smart and more worried about
remembering to think about this at all.


> > > If you wanted to make sure the whitelist was obeyed, I think I'd
> prefer a
> > > presubmit of some sort over comments.
> >
> > I don't know how I would do this in a presubmit, as it is specific to
> > certain types (PaintOp subtypes), other than some elaborate multiline c++
> > parsing which I don't want to write.
>
> Yeah. I figured most of these PaintOps had a pretty regular structure (all
> members separated by one line of whitespace before a "^};"). I know nobody
> wants to parse C++ in Python, but it was a thought for automation since
> this was
> all pretty regular.
>
> My other thoughts were like declare all the members with some sort of
> "decltype(IsSafeType<SkRect>::value) rect" template nonsense, but that's
> suuuuuper wordy too.
>

That's an interesting thought! I can try it and see what it looks like.


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

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

[danakj, 2017-06-01 15:49:11]

Description was changed from

==========
cc: Audit and static_assert the types in PaintOpBuffer can be moved.

This verifies that we can move the types stored in PaintOpBuffer to
a new memory address without calling a move constructor or destructor,
by doing memcpy() and free()ing the old memory.

std::is_trivially_copyable() types satisfy this since they have a
trivial move constructor, but it requires a lot of other things we do
not require since we don't keep around both copies.

std::is_trivially_move_constructible() should satisfy this also, as we
are conceptually moving everything, but this is not available to us on
all platforms right now. I have left a TODO to this effect. It actually
provides more than we need as well, as it has built in the assumption
that we will call the destructor on the source object. sk_sp<> is an
example that does not satisfy this condition since it nulls out the
moved-from object. Since we don't ever use/destroy the moved-from
object this is not important for us, and sk_sp functions correctly when
we memcpy() and free() it, essentially moving its memory address.

Our requirements are that the object does not need to modify itself
when changing its address. In practice this generally means that it owns
no pointers to itself directly or indirectly. A bad example is that
std::vector can have a pointer back to the vector from its internal
buffer.

As of this patch no types in PaintOpBuffer are problematic, but this
adds static_asserts for is_trivially_copyable where possible, comments
on known trivial types (such as bool), and explanations on other types
that we hope will be maintained /o\.

R=enne@chromium.org
BUG=671433
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
cc: Audit and static_assert the types in PaintOpBuffer can be moved.

This verifies that we can move the types stored in PaintOpBuffer to
a new memory address without calling a move constructor or destructor,
by doing memcpy() and free()ing the old memory.

std::is_trivially_copyable() types satisfy this since they have a
trivial move constructor, but it requires a lot of other things we do
not require since we don't keep around both copies.

std::is_trivially_move_constructible() should satisfy this also, as we
are conceptually moving everything, but this is not available to us on
all platforms right now. I have left a TODO to this effect. It actually
provides more than we need as well, as it has built in the assumption
that we will call the destructor on the source object. sk_sp<> is an
example that does not satisfy this condition since it nulls out the
moved-from object. Since we don't ever use/destroy the moved-from
object this is not important for us, and sk_sp functions correctly when
we memcpy() and free() it, essentially moving its memory address.

Our requirements are that the object does not need to modify itself
when changing its address. In practice this generally means that it owns
no pointers to itself directly or indirectly. A bad example is that
std::vector can have a pointer back to the vector from its internal
buffer.

As of this patch no types in PaintOpBuffer are problematic. But now all
non-primitive types in PaintOps go through SafePaintOpData which does
the following:
- Adds static_asserts for is_trivially_copyable where possible.
- Has explanations on other types that we hope will be maintained /o\.


R=enne@chromium.org
BUG=671433
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[danakj, 2017-06-01 15:49:11]

Description was changed from

==========
cc: Audit and static_assert the types in PaintOpBuffer can be moved.

This verifies that we can move the types stored in PaintOpBuffer to
a new memory address without calling a move constructor or destructor,
by doing memcpy() and free()ing the old memory.

std::is_trivially_copyable() types satisfy this since they have a
trivial move constructor, but it requires a lot of other things we do
not require since we don't keep around both copies.

std::is_trivially_move_constructible() should satisfy this also, as we
are conceptually moving everything, but this is not available to us on
all platforms right now. I have left a TODO to this effect. It actually
provides more than we need as well, as it has built in the assumption
that we will call the destructor on the source object. sk_sp<> is an
example that does not satisfy this condition since it nulls out the
moved-from object. Since we don't ever use/destroy the moved-from
object this is not important for us, and sk_sp functions correctly when
we memcpy() and free() it, essentially moving its memory address.

Our requirements are that the object does not need to modify itself
when changing its address. In practice this generally means that it owns
no pointers to itself directly or indirectly. A bad example is that
std::vector can have a pointer back to the vector from its internal
buffer.

As of this patch no types in PaintOpBuffer are problematic, but this
adds static_asserts for is_trivially_copyable where possible, comments
on known trivial types (such as bool), and explanations on other types
that we hope will be maintained /o\.

R=enne@chromium.org
BUG=671433
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
cc: Audit and static_assert the types in PaintOpBuffer can be moved.

This verifies that we can move the types stored in PaintOpBuffer to
a new memory address without calling a move constructor or destructor,
by doing memcpy() and free()ing the old memory.

std::is_trivially_copyable() types satisfy this since they have a
trivial move constructor, but it requires a lot of other things we do
not require since we don't keep around both copies.

std::is_trivially_move_constructible() should satisfy this also, as we
are conceptually moving everything, but this is not available to us on
all platforms right now. I have left a TODO to this effect. It actually
provides more than we need as well, as it has built in the assumption
that we will call the destructor on the source object. sk_sp<> is an
example that does not satisfy this condition since it nulls out the
moved-from object. Since we don't ever use/destroy the moved-from
object this is not important for us, and sk_sp functions correctly when
we memcpy() and free() it, essentially moving its memory address.

Our requirements are that the object does not need to modify itself
when changing its address. In practice this generally means that it owns
no pointers to itself directly or indirectly. A bad example is that
std::vector can have a pointer back to the vector from its internal
buffer.

As of this patch no types in PaintOpBuffer are problematic. But now all
non-primitive types in PaintOps go through SafePaintOpData which does
the following:
- Adds static_asserts for is_trivially_copyable where possible.
- Has explanations on other types that we hope will be maintained /o\.


R=enne@chromium.org
BUG=671433
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[danakj, 2017-06-01 15:49:16]

PTAL

[danakj, 2017-06-01 15:49:19]

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

[danakj, 2017-06-01 15:49:44]

Description was changed from

==========
cc: Audit and static_assert the types in PaintOpBuffer can be moved.

This verifies that we can move the types stored in PaintOpBuffer to
a new memory address without calling a move constructor or destructor,
by doing memcpy() and free()ing the old memory.

std::is_trivially_copyable() types satisfy this since they have a
trivial move constructor, but it requires a lot of other things we do
not require since we don't keep around both copies.

std::is_trivially_move_constructible() should satisfy this also, as we
are conceptually moving everything, but this is not available to us on
all platforms right now. I have left a TODO to this effect. It actually
provides more than we need as well, as it has built in the assumption
that we will call the destructor on the source object. sk_sp<> is an
example that does not satisfy this condition since it nulls out the
moved-from object. Since we don't ever use/destroy the moved-from
object this is not important for us, and sk_sp functions correctly when
we memcpy() and free() it, essentially moving its memory address.

Our requirements are that the object does not need to modify itself
when changing its address. In practice this generally means that it owns
no pointers to itself directly or indirectly. A bad example is that
std::vector can have a pointer back to the vector from its internal
buffer.

As of this patch no types in PaintOpBuffer are problematic. But now all
non-primitive types in PaintOps go through SafePaintOpData which does
the following:
- Adds static_asserts for is_trivially_copyable where possible.
- Has explanations on other types that we hope will be maintained /o\.


R=enne@chromium.org
BUG=671433
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
cc: Audit and static_assert the types in PaintOpBuffer can be moved.

This verifies that we can move the types stored in PaintOpBuffer to
a new memory address without calling a move constructor or destructor,
by doing memcpy() and free()ing the old memory.

std::is_trivially_copyable() types satisfy this since they have a
trivial move constructor, but it requires a lot of other things we do
not require since we don't keep around both copies.

std::is_trivially_move_constructible() should satisfy this also, as we
are conceptually moving everything, but this is not available to us on
all platforms right now. I have left a TODO to this effect. It actually
provides more than we need as well, as it has built in the assumption
that we will call the destructor on the source object. sk_sp<> is an
example that does not satisfy this condition since it nulls out the
moved-from object. Since we don't ever use/destroy the moved-from
object this is not important for us, and sk_sp functions correctly when
we memcpy() and free() it, essentially moving its memory address.

Our requirements are that the object does not need to modify itself
when changing its address. In practice this generally means that it owns
no pointers to itself directly or indirectly. A bad example is that
std::vector can have a pointer back to the vector from its internal
buffer.

As of this patch no types in PaintOpBuffer are problematic. But now all
non-primitive types in PaintOps go through SafePaintOpData which does
the following:
- Adds static_asserts for is_trivially_copyable where possible.
- Has explanations on other types that we hope will be maintained /o\.

R=enne@chromium.org
BUG=671433
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[commit-bot: I haz the power, 2017-06-01 15:49:52]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[danakj, 2017-06-01 15:51:49]

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

[commit-bot: I haz the power, 2017-06-01 15:52:22]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[enne (OOO), 2017-06-01 16:04:58]

https://codereview.chromium.org/2899483002/diff/80001/cc/paint/paint_op_buffer.h
File cc/paint/paint_op_buffer.h (right):

https://codereview.chromium.org/2899483002/diff/80001/cc/paint/paint_op_buffe...
cc/paint/paint_op_buffer.h:802: float degrees;
If you think we should get rid of SkScalar types, I think there's a larger
change to be made elsewhere in other APIs too.  Changing it only here seems
inconsistent, because we go SkScalar -> float -> SkScalar again.  Sure, it's
"really" a float all the way through, but still.

I'd weakly like to leave this as-is in this patch, but could be convinced
otherwise.

[commit-bot: I haz the power, 2017-06-01 17:42:08]

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

[commit-bot: I haz the power, 2017-06-01 17:42:09]

Dry run: This issue passed the CQ dry run.

[danakj, 2017-06-01 18:01:18]

https://codereview.chromium.org/2899483002/diff/80001/cc/paint/paint_op_buffer.h
File cc/paint/paint_op_buffer.h (right):

https://codereview.chromium.org/2899483002/diff/80001/cc/paint/paint_op_buffe...
cc/paint/paint_op_buffer.h:802: float degrees;
On 2017/06/01 16:04:58, enne wrote:
> If you think we should get rid of SkScalar types, I think there's a larger
> change to be made elsewhere in other APIs too.  Changing it only here seems
> inconsistent, because we go SkScalar -> float -> SkScalar again.  Sure, it's
> "really" a float all the way through, but still.
> 
> I'd weakly like to leave this as-is in this patch, but could be convinced
> otherwise.

I thought it made sense to use float here because it's a known primitive and it
is actually a float. I could use SafePaintOpData<SkScalar> but that seemed meh
to me. I'd like to make all our APIs use float instead, but this seemed like a
good start.

[enne (OOO), 2017-06-01 18:07:33]

https://codereview.chromium.org/2899483002/diff/80001/cc/paint/paint_op_buffer.h
File cc/paint/paint_op_buffer.h (right):

https://codereview.chromium.org/2899483002/diff/80001/cc/paint/paint_op_buffe...
cc/paint/paint_op_buffer.h:802: float degrees;
On 2017/06/01 at 18:01:17, danakj wrote:
> On 2017/06/01 16:04:58, enne wrote:
> > If you think we should get rid of SkScalar types, I think there's a larger
> > change to be made elsewhere in other APIs too.  Changing it only here seems
> > inconsistent, because we go SkScalar -> float -> SkScalar again.  Sure, it's
> > "really" a float all the way through, but still.
> > 
> > I'd weakly like to leave this as-is in this patch, but could be convinced
> > otherwise.
> 
> I thought it made sense to use float here because it's a known primitive and
it is actually a float. I could use SafePaintOpData<SkScalar> but that seemed
meh to me. I'd like to make all our APIs use float instead, but this seemed like
a good start.

I didn't see anything wrong with SafePaintOpData<SkScalar> (especially since
then it's clear that everything goes through SafePaintOpData).  Do you think we
shouldn't do SafePaintOpData<float> for consistency?

[vmpstr, 2017-06-01 18:16:17]

vmpstr@chromium.org changed reviewers:
+ vmpstr@chromium.org

[vmpstr, 2017-06-01 18:16:17]

Like your description says, I feel like this patch is adding static asserts for
types that are either enums or are likely to never change (ie SkRect or
SkMatrix), and the only enforcement for types that _are_ likely to change is
comments (ie PaintImage, which we are likely to add more things into; PaintFlags
which are likely to mutate as we evolve this code).

I'm not sure what this is buying us to be honest. Do you have a strong reason
for this?

[danakj, 2017-06-01 18:54:05]

On Thu, Jun 1, 2017 at 2:07 PM, enne@chromium.org via
codereview.chromium.org <reply@chromiumcodereview-hr.appspotmail.com> wrote:

>
> https://codereview.chromium.org/2899483002/diff/80001/cc/
> paint/paint_op_buffer.h
> File cc/paint/paint_op_buffer.h (right):
>
> https://codereview.chromium.org/2899483002/diff/80001/cc/
> paint/paint_op_buffer.h#newcode802
> cc/paint/paint_op_buffer.h:802: float degrees;
> On 2017/06/01 at 18:01:17, danakj wrote:
> > On 2017/06/01 16:04:58, enne wrote:
> > > If you think we should get rid of SkScalar types, I think there's a
> larger
> > > change to be made elsewhere in other APIs too. Changing it only
> here seems
> > > inconsistent, because we go SkScalar -> float -> SkScalar again.
> Sure, it's
> > > "really" a float all the way through, but still.
> > >
> > > I'd weakly like to leave this as-is in this patch, but could be
> convinced
> > > otherwise.
> >
> > I thought it made sense to use float here because it's a known
> primitive and it is actually a float. I could use
> SafePaintOpData<SkScalar> but that seemed meh to me. I'd like to make
> all our APIs use float instead, but this seemed like a good start.
>
> I didn't see anything wrong with SafePaintOpData<SkScalar> (especially
> since then it's clear that everything goes through SafePaintOpData). Do
> you think we shouldn't do SafePaintOpData<float> for consistency?
>

Ok used SafePaintOpData<SkScalar> now

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

[danakj, 2017-06-01 19:00:33]

On Thu, Jun 1, 2017 at 2:16 PM, <vmpstr@chromium.org> wrote:

> Like your description says, I feel like this patch is adding static
> asserts for
> types that are either enums or are likely to never change (ie SkRect or
> SkMatrix), and the only enforcement for types that _are_ likely to change
> is
> comments (ie PaintImage, which we are likely to add more things into;
> PaintFlags
> which are likely to mutate as we evolve this code).
>
> I'm not sure what this is buying us to be honest. Do you have a strong
> reason
> for this?
>

Because we already did this wrong in ContiguousContainer with vector. I
want reviewers of the future to know there is something to look out for,
what the constraints are, and make it as hard as possible to add something
without thinking about it, so both authors and reviewers notice.

The hardest one here is PaintFlags which is an SkPaint internally. As we
unravel that to be less pointery, it will be easy to break this assumption
and we need to be careful, for example. I do think this should be well
documented. I can add comments in PaintFlags if we add something beside the
SkPaint. I think inside skia we're pretty safe cuz they sk_sp everything it
seems. But if something started crashing and we noticed it, we'd have clear
comments explaining things at least back here.

I think we're naively safe right now because all the complex types are
stored in refptrs. But that's explicitly a non-goal so as we unwrap those
we need to think about each one carefully.

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

[danakj, 2017-06-01 19:17:37]

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

[commit-bot: I haz the power, 2017-06-01 19:18:14]

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, 2017-06-01 19:29:49]

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

[commit-bot: I haz the power, 2017-06-01 19:29:50]

Dry run: Try jobs failed on following builders:
  linux_trusty_blink_rel on master.tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_trusty_blink_rel/b...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  linux_chromium_headless_rel on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[danakj, 2017-06-01 19:33:15]

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

[commit-bot: I haz the power, 2017-06-01 19:33:53]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[vmpstr, 2017-06-01 20:11:27]

On 2017/06/01 19:00:33, danakj wrote:
> On Thu, Jun 1, 2017 at 2:16 PM, <mailto:vmpstr@chromium.org> wrote:
> 
> > Like your description says, I feel like this patch is adding static
> > asserts for
> > types that are either enums or are likely to never change (ie SkRect or
> > SkMatrix), and the only enforcement for types that _are_ likely to change
> > is
> > comments (ie PaintImage, which we are likely to add more things into;
> > PaintFlags
> > which are likely to mutate as we evolve this code).
> >
> > I'm not sure what this is buying us to be honest. Do you have a strong
> > reason
> > for this?
> >
> 
> Because we already did this wrong in ContiguousContainer with vector. I
> want reviewers of the future to know there is something to look out for,
> what the constraints are, and make it as hard as possible to add something
> without thinking about it, so both authors and reviewers notice.
> 
> The hardest one here is PaintFlags which is an SkPaint internally. As we
> unravel that to be less pointery, it will be easy to break this assumption
> and we need to be careful, for example. I do think this should be well
> documented. I can add comments in PaintFlags if we add something beside the
> SkPaint. I think inside skia we're pretty safe cuz they sk_sp everything it
> seems. But if something started crashing and we noticed it, we'd have clear
> comments explaining things at least back here.
> 
> I think we're naively safe right now because all the complex types are
> stored in refptrs. But that's explicitly a non-goal so as we unwrap those
> we need to think about each one carefully.
> 

I'm not against plenty of documentation that explains at length why we're doing
what we're doing, and why we should be very careful about modifying the types
used in a PaintOpBuffer. In fact, I like the comments that you put throughout
and I think we should put them in every paint class that is going to be a part
of a paint op buffer.

My concern is that SafePaintOpData introduced by this patch does a good job
ensuring that types that are very unlikely to change meet this criteria, but it
does nothing for types we should be most worried about. And this comes at a cost
of added complexity to paint_op_buffer. It feels a bit like security by
complexity, as in authors will be more reluctant to change code that is higher
complexity; I'm sure that's not the intent though. 

This came up when we were thinking of adding a vector of frames to PaintImage,
which would violate the assumptions you've stated in paint_op_buffer. The author
wasn't aware of these requirements, and honestly I wouldn't have been if it
wasn't for a comment in PaintImage. In other words, without the comment in
PaintImage, there is nothing that would raise a red flag for either the author
or the reviewer. So, that comment is good. SafePaintOpData wouldn't have
precluded the change though, since (1) it's in a seemingly unrelated file, (2)
it would pass since it has no checks, and as a result (3) would provide a false
sense of security since it looks like we're checking something with it, and it
compiles, so looks like the change is good.

[danakj, 2017-06-01 20:18:49]

On Thu, Jun 1, 2017 at 4:11 PM, <vmpstr@chromium.org> wrote:

> On 2017/06/01 19:00:33, danakj wrote:
>
> > On Thu, Jun 1, 2017 at 2:16 PM, <mailto:vmpstr@chromium.org> wrote:
> >
> > > Like your description says, I feel like this patch is adding static
> > > asserts for
> > > types that are either enums or are likely to never change (ie SkRect or
> > > SkMatrix), and the only enforcement for types that _are_ likely to
> change
> > > is
> > > comments (ie PaintImage, which we are likely to add more things into;
> > > PaintFlags
> > > which are likely to mutate as we evolve this code).
> > >
> > > I'm not sure what this is buying us to be honest. Do you have a strong
> > > reason
> > > for this?
> > >
> >
> > Because we already did this wrong in ContiguousContainer with vector. I
> > want reviewers of the future to know there is something to look out for,
> > what the constraints are, and make it as hard as possible to add
> something
> > without thinking about it, so both authors and reviewers notice.
> >
> > The hardest one here is PaintFlags which is an SkPaint internally. As we
> > unravel that to be less pointery, it will be easy to break this
> assumption
> > and we need to be careful, for example. I do think this should be well
> > documented. I can add comments in PaintFlags if we add something beside
> the
> > SkPaint. I think inside skia we're pretty safe cuz they sk_sp everything
> it
> > seems. But if something started crashing and we noticed it, we'd have
> clear
> > comments explaining things at least back here.
> >
> > I think we're naively safe right now because all the complex types are
> > stored in refptrs. But that's explicitly a non-goal so as we unwrap those
> > we need to think about each one carefully.
> >
>
> I'm not against plenty of documentation that explains at length why we're
> doing
> what we're doing, and why we should be very careful about modifying the
> types
> used in a PaintOpBuffer. In fact, I like the comments that you put
> throughout
> and I think we should put them in every paint class that is going to be a
> part
> of a paint op buffer.
>
> My concern is that SafePaintOpData introduced by this patch does a good job
> ensuring that types that are very unlikely to change meet this criteria,
> but it
> does nothing for types we should be most worried about. And this comes at
> a cost
> of added complexity to paint_op_buffer. It feels a bit like security by
> complexity, as in authors will be more reluctant to change code that is
> higher
> complexity; I'm sure that's not the intent though.
>

You're right that this doesn't guard the internals of the types, and
there's 2 ways I can think of to address that.
1) Comments as I've done for PaintImage.
2) Force types to expose something, and that thing can check the ok-ness of
its members. This moves things down one more level.

Would 2 do better by you?


>
> This came up when we were thinking of adding a vector of frames to
> PaintImage,
> which would violate the assumptions you've stated in paint_op_buffer. The
> author
> wasn't aware of these requirements, and honestly I wouldn't have been if it
> wasn't for a comment in PaintImage. In other words, without the comment in
> PaintImage, there is nothing that would raise a red flag for either the
> author
> or the reviewer. So, that comment is good. SafePaintOpData wouldn't have
> precluded the change though, since (1) it's in a seemingly unrelated file,
> (2)
> it would pass since it has no checks, and as a result (3) would provide a
> false
> sense of security since it looks like we're checking something with it,
> and it
> compiles, so looks like the change is good.
>

These checks as is only protect against adding things to PaintOp directly,
which I think we do want. We could call something on nested types that we
control, and have them recursively do the same sort of thing.

For skia-controlled types we have to just kinda hope really, it's at best a
breadcrumb trail if things go terribly wrong. But since they use sk_sp
liberally it's probably fine.


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

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

[vmpstr, 2017-06-01 20:39:30]

> 
> You're right that this doesn't guard the internals of the types, and
> there's 2 ways I can think of to address that.
> 1) Comments as I've done for PaintImage.
> 2) Force types to expose something, and that thing can check the ok-ness of
> its members. This moves things down one more level.
> 
> Would 2 do better by you?
> 

Since these are compile-time checks, you can just add static asserts for each
member of the corresponding types. Ie,

class PaintFoo {
 ...
 private:
  static_assert(base::is_trivially_copyable<Bar>::value, "");
  Bar bar_;

  // base::is_trivially_copyable<sk_sp<Baz>>::value won't pass, but it's ok,
because ...
  sk_sp<Baz> baz_;
};

That looks like it's a better protection than either 1 or 2 (I do want 1
though).

> 
> >
> > This came up when we were thinking of adding a vector of frames to
> > PaintImage,
> > which would violate the assumptions you've stated in paint_op_buffer. The
> > author
> > wasn't aware of these requirements, and honestly I wouldn't have been if it
> > wasn't for a comment in PaintImage. In other words, without the comment in
> > PaintImage, there is nothing that would raise a red flag for either the
> > author
> > or the reviewer. So, that comment is good. SafePaintOpData wouldn't have
> > precluded the change though, since (1) it's in a seemingly unrelated file,
> > (2)
> > it would pass since it has no checks, and as a result (3) would provide a
> > false
> > sense of security since it looks like we're checking something with it,
> > and it
> > compiles, so looks like the change is good.
> >
> 
> These checks as is only protect against adding things to PaintOp directly,
> which I think we do want. We could call something on nested types that we
> control, and have them recursively do the same sort of thing.

For one I'm not sure how much we'd be adding to this. That is, a clip rect will
always have a rect kind of thing... Maybe you have some thoughts on other types
that would eventually be here. This also looks like at best a hope that a person
adding something will notice most neighboring members are wrapped... and then
all they really have to do is add another specialization that doesn't do
anything. I guess what I'm saying is that unless the reviewer is one of the
people that is already aware of these requirements, there are very few guards to
hint at the requirements (save for maybe the comments).

Maybe at least don't specialize every type that we have, and have a default
implementation that does a static assert (with a message). That will trigger an
error of a new non trivially copyable type that will explain what to do.

Can you also explain why we have this requirement (near the block where you
state that we do have this requirement). As it stands now, it's not clear what
the use case would be where the requirement is important. Furthermore, I think
it's important to note that simply memcpy-ing a ref counted pointer is not
enough, the original copy _must_ be deleted without destructors. You allude a
little bit about this in the comment, but I feel like it could use a bit more
WARNING type of things around.

> 
> For skia-controlled types we have to just kinda hope really, it's at best a
> breadcrumb trail if things go terribly wrong. But since they use sk_sp
> liberally it's probably fine.
>

[danakj, 2017-06-01 21:08:40]

On Thu, Jun 1, 2017 at 4:39 PM, <vmpstr@chromium.org> wrote:

> >
> > You're right that this doesn't guard the internals of the types, and
> > there's 2 ways I can think of to address that.
> > 1) Comments as I've done for PaintImage.
> > 2) Force types to expose something, and that thing can check the ok-ness
> of
> > its members. This moves things down one more level.
> >
> > Would 2 do better by you?
> >
>
> Since these are compile-time checks, you can just add static asserts for
> each
> member of the corresponding types. Ie,
>
> class PaintFoo {
> ...
> private:
> static_assert(base::is_trivially_copyable<Bar>::value, "");
> Bar bar_;
>
> // base::is_trivially_copyable<sk_sp<Baz>>::value won't pass, but it's ok,
> because ...
> sk_sp<Baz> baz_;
> };
>
> That looks like it's a better protection than either 1 or 2 (I do want 1
> though).
>

This is the kinda verbosity we avoided in the PaintOps. I don't mind doing
this, but I wonder if we should move the template up and use it here too?

something like cc::IsMemMovable? We could specialize on is_enum to avoid
having to name them all for instance..

>
>
> >
> > >
> > > This came up when we were thinking of adding a vector of frames to
> > > PaintImage,
> > > which would violate the assumptions you've stated in paint_op_buffer.
> The
> > > author
> > > wasn't aware of these requirements, and honestly I wouldn't have been
> if it
> > > wasn't for a comment in PaintImage. In other words, without the
> comment in
> > > PaintImage, there is nothing that would raise a red flag for either the
> > > author
> > > or the reviewer. So, that comment is good. SafePaintOpData wouldn't
> have
> > > precluded the change though, since (1) it's in a seemingly unrelated
> file,
> > > (2)
> > > it would pass since it has no checks, and as a result (3) would
> provide a
> > > false
> > > sense of security since it looks like we're checking something with it,
> > > and it
> > > compiles, so looks like the change is good.
> > >
> >
> > These checks as is only protect against adding things to PaintOp
> directly,
> > which I think we do want. We could call something on nested types that we
> > control, and have them recursively do the same sort of thing.
>
> For one I'm not sure how much we'd be adding to this. That is, a clip rect
> will
> always have a rect kind of thing... Maybe you have some thoughts on other
> types
> that would eventually be here.
>

Any data structure with a pointer in it could have this problem. Right now
they are all behind sk_sp<>s but in the future we will unwrap these to make
the stuff shared-memory-able. As we do so we need to look at the internals
of each type, and this is all a reminder to do so. Currently that's all
wrapped up inside PaintFlags but we will unwrap that over time and move
things out to the ops that use it, and drop refcounting for stuff. I can
imagine intermediate states that have pointers and then we crash a lot with
confusing stacks.

My goal is mostly to give reviewers (and authors) lots of context to make
it harder to mess this up.


> This also looks like at best a hope that a person
> adding something will notice most neighboring members are wrapped... and
> then
> all they really have to do is add another specialization that doesn't do
> anything.
>

Yep, I depend on cargo culting and reviewers reading the file. I don't have
a better proposition tho.


> I guess what I'm saying is that unless the reviewer is one of the
> people that is already aware of these requirements, there are very few
> guards to
> hint at the requirements (save for maybe the comments).
>

I am hoping that there is enough context for a reviewer who is not aware to
become aware when looking at a patch here. Do you think that's not the
case? Doing nothing would require them to be already aware. A comment would
give them a place to find it if they happened to. The templates will force
someone to write a new specialization beside a bunch of others that explain
things. At that point the reviewer gets a lot more to be like "hmm" about,
which is the goal.


> Maybe at least don't specialize every type that we have, and have a default
> implementation that does a static assert (with a message). That will
> trigger an
> error of a new non trivially copyable type that will explain what to do.
>

How do I not specialize each type, but still get an error for an unknown
type?


> Can you also explain why we have this requirement (near the block where you
> state that we do have this requirement).
>

// It is a requirement for every field in a PaintOp that we can memcpy it
to
// another place in memory and free the only one (without calling any

// constructors or destructors).

Is the comment on PaintOp. I will copy that roughly up to the template.


> As it stands now, it's not clear what
> the use case would be where the requirement is important.
>

Do you think that comment bit is not clear or just its position?


> Furthermore, I think
> it's important to note that simply memcpy-ing a ref counted pointer is not
> enough, the original copy _must_ be deleted without destructors. You
> allude a
> little bit about this in the comment, but I feel like it could use a bit
> more
> WARNING type of things around.
>

// sk_sp<T> pointers behave if their memory address is changed

Is the comment I have for sk_sp to explain that we're moving the address of
the object (memcpy+free) not memcpying. I can add some WARNING but I'm not
sure what to put. WARNING that we will free not just memcpy? That seems to
be strictly better though, as it's a moving address instead of skipping a
constructor but having an object suddenly, so warning about that feels
weird. Or would you like some WARNING in the PaintOp comment where I
explain the whole thing? I'll throw a WARNING at the top of that and see
what you think.


>
>
> >
> > For skia-controlled types we have to just kinda hope really, it's at
> best a
> > breadcrumb trail if things go terribly wrong. But since they use sk_sp
> > liberally it's probably fine.
> >
>
>
> https://codereview.chromium.org/2899483002/
>

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

[danakj, 2017-06-01 21:08:49]

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

[commit-bot: I haz the power, 2017-06-01 21:09:07]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[danakj, 2017-06-01 21:22:09]

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

[commit-bot: I haz the power, 2017-06-01 21:22:58]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[vmpstr, 2017-06-01 21:26:40]

On 2017/06/01 21:08:40, danakj wrote:
> On Thu, Jun 1, 2017 at 4:39 PM, <mailto:vmpstr@chromium.org> wrote:
> 
> > >
> > > You're right that this doesn't guard the internals of the types, and
> > > there's 2 ways I can think of to address that.
> > > 1) Comments as I've done for PaintImage.
> > > 2) Force types to expose something, and that thing can check the ok-ness
> > of
> > > its members. This moves things down one more level.
> > >
> > > Would 2 do better by you?
> > >
> >
> > Since these are compile-time checks, you can just add static asserts for
> > each
> > member of the corresponding types. Ie,
> >
> > class PaintFoo {
> > ...
> > private:
> > static_assert(base::is_trivially_copyable<Bar>::value, "");
> > Bar bar_;
> >
> > // base::is_trivially_copyable<sk_sp<Baz>>::value won't pass, but it's ok,
> > because ...
> > sk_sp<Baz> baz_;
> > };
> >
> > That looks like it's a better protection than either 1 or 2 (I do want 1
> > though).
> >
> 
> This is the kinda verbosity we avoided in the PaintOps. I don't mind doing
> this, but I wonder if we should move the template up and use it here too?
> 
> something like cc::IsMemMovable? We could specialize on is_enum to avoid
> having to name them all for instance..
> 
> >
> >
> > >
> > > >
> > > > This came up when we were thinking of adding a vector of frames to
> > > > PaintImage,
> > > > which would violate the assumptions you've stated in paint_op_buffer.
> > The
> > > > author
> > > > wasn't aware of these requirements, and honestly I wouldn't have been
> > if it
> > > > wasn't for a comment in PaintImage. In other words, without the
> > comment in
> > > > PaintImage, there is nothing that would raise a red flag for either the
> > > > author
> > > > or the reviewer. So, that comment is good. SafePaintOpData wouldn't
> > have
> > > > precluded the change though, since (1) it's in a seemingly unrelated
> > file,
> > > > (2)
> > > > it would pass since it has no checks, and as a result (3) would
> > provide a
> > > > false
> > > > sense of security since it looks like we're checking something with it,
> > > > and it
> > > > compiles, so looks like the change is good.
> > > >
> > >
> > > These checks as is only protect against adding things to PaintOp
> > directly,
> > > which I think we do want. We could call something on nested types that we
> > > control, and have them recursively do the same sort of thing.
> >
> > For one I'm not sure how much we'd be adding to this. That is, a clip rect
> > will
> > always have a rect kind of thing... Maybe you have some thoughts on other
> > types
> > that would eventually be here.
> >
> 
> Any data structure with a pointer in it could have this problem. Right now
> they are all behind sk_sp<>s but in the future we will unwrap these to make
> the stuff shared-memory-able. As we do so we need to look at the internals
> of each type, and this is all a reminder to do so. Currently that's all
> wrapped up inside PaintFlags but we will unwrap that over time and move
> things out to the ops that use it, and drop refcounting for stuff. I can
> imagine intermediate states that have pointers and then we crash a lot with
> confusing stacks.
> 
> My goal is mostly to give reviewers (and authors) lots of context to make
> it harder to mess this up.

I understand and share this goal. What I want in addition is to ensure that the
author and the reviewer understand why we have this and realize that if the code
compiles it doesn't mean that they haven't messed up. The patch as is makes it
feel like as long as you can successfully compile, it's good.

> 
> 
> > This also looks like at best a hope that a person
> > adding something will notice most neighboring members are wrapped... and
> > then
> > all they really have to do is add another specialization that doesn't do
> > anything.
> >
> 
> Yep, I depend on cargo culting and reviewers reading the file. I don't have
> a better proposition tho.
> 
> 
> > I guess what I'm saying is that unless the reviewer is one of the
> > people that is already aware of these requirements, there are very few
> > guards to
> > hint at the requirements (save for maybe the comments).
> >
> 
> I am hoping that there is enough context for a reviewer who is not aware to
> become aware when looking at a patch here. Do you think that's not the
> case? Doing nothing would require them to be already aware. A comment would
> give them a place to find it if they happened to. The templates will force
> someone to write a new specialization beside a bunch of others that explain
> things. At that point the reviewer gets a lot more to be like "hmm" about,
> which is the goal.
> 
> 
> > Maybe at least don't specialize every type that we have, and have a default
> > implementation that does a static assert (with a message). That will
> > trigger an
> > error of a new non trivially copyable type that will explain what to do.
> >
> 
> How do I not specialize each type, but still get an error for an unknown
> type?

I'm not sure why you want an error if the type is trivially copyable. That seems
like just extra effort on the author's part. I'm proposing a default
implementation that static asserts for this condition, so any previously unknown
type will be checked and only fail if it's not trivially copyable. The failure
should provide a link to some sort of documentation. The documentation, in turn,
should explain why we require this condition and what criteria has to be met
before the author writes an exception specialization (like for PaintImage and
PaintFlags).  

> 
> 
> > Can you also explain why we have this requirement (near the block where you
> > state that we do have this requirement).
> >
> 
> // It is a requirement for every field in a PaintOp that we can memcpy it
> to
> // another place in memory and free the only one (without calling any
> 
> // constructors or destructors).
> 
> Is the comment on PaintOp. I will copy that roughly up to the template.
> 
> 
> > As it stands now, it's not clear what
> > the use case would be where the requirement is important.
> >
> 
> Do you think that comment bit is not clear or just its position?

The comment position is fine. I'm only saying that the comment is stating that
we have an assumption, but doesn't seem to provide a reason why we need that
assumption. I just want a "why" blurb not just "what".

> 
> 
> > Furthermore, I think
> > it's important to note that simply memcpy-ing a ref counted pointer is not
> > enough, the original copy _must_ be deleted without destructors. You
> > allude a
> > little bit about this in the comment, but I feel like it could use a bit
> > more
> > WARNING type of things around.
> >
> 
> // sk_sp<T> pointers behave if their memory address is changed
> 
> Is the comment I have for sk_sp to explain that we're moving the address of
> the object (memcpy+free) not memcpying. I can add some WARNING but I'm not
> sure what to put. WARNING that we will free not just memcpy? That seems to
> be strictly better though, as it's a moving address instead of skipping a
> constructor but having an object suddenly, so warning about that feels
> weird. Or would you like some WARNING in the PaintOp comment where I
> explain the whole thing? I'll throw a WARNING at the top of that and see
> what you think.
> 

I think if you add the "why" above it might become more clear. I want it to be
very clear that you can't just memcpy the whole thing and then use both copies,
since you're going to double free ref counted pointers.

> 
> >
> >
> > >
> > > For skia-controlled types we have to just kinda hope really, it's at
> > best a
> > > breadcrumb trail if things go terribly wrong. But since they use sk_sp
> > > liberally it's probably fine.
> > >
> >
> >
> > https://codereview.chromium.org/2899483002/
> >
> 
> -- 
> 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 mailto:chromium-reviews+unsubscribe@chromium.org.

[enne (OOO), 2017-06-01 21:28:57]

Another thought I had is that serialization logic subsumes some of this.  Any
type with pointers is going to need special serialization logic to handle it.  I
am already writing a PaintOpWriter class that has methods like this:

  void Write(uint8_t data);
  void Write(const SkRect& rect);
  void Write(const SkIRect& rect);
  void Write(const SkRRect& rect);

  void Write(const SkPath& path);
  void Write(const cc::PaintFlags& flags);
  void Write(const PaintImage& image);
  void Write(const sk_sp<SkData>& data);
  void Write(const sk_sp<SkTextBlob>& blob);

...which is kind of the same thing as what you're doing.  If anybody added a
std::vector, there'd likewise need to be special serialization logic.  Right
now, everything is implicit because the buffer gets realloc'd, but I think it is
already going to be explicit, and so extensive warnings inside of PaintOpBuffer
become kind of redundant to that.

[danakj, 2017-06-01 21:43:46]

On Thu, Jun 1, 2017 at 5:26 PM, <vmpstr@chromium.org> wrote:

> On 2017/06/01 21:08:40, danakj wrote:
>
> > On Thu, Jun 1, 2017 at 4:39 PM, <mailto:vmpstr@chromium.org> wrote:
> >
> > > >
> > > > You're right that this doesn't guard the internals of the types, and
> > > > there's 2 ways I can think of to address that.
> > > > 1) Comments as I've done for PaintImage.
> > > > 2) Force types to expose something, and that thing can check the
> ok-ness
> > > of
> > > > its members. This moves things down one more level.
> > > >
> > > > Would 2 do better by you?
> > > >
> > >
> > > Since these are compile-time checks, you can just add static asserts
> for
> > > each
> > > member of the corresponding types. Ie,
> > >
> > > class PaintFoo {
> > > ...
> > > private:
> > > static_assert(base::is_trivially_copyable<Bar>::value, "");
> > > Bar bar_;
> > >
> > > // base::is_trivially_copyable<sk_sp<Baz>>::value won't pass, but
> it's ok,
> > > because ...
> > > sk_sp<Baz> baz_;
> > > };
> > >
> > > That looks like it's a better protection than either 1 or 2 (I do want
> 1
> > > though).
> > >
> >
> > This is the kinda verbosity we avoided in the PaintOps. I don't mind
> doing
> > this, but I wonder if we should move the template up and use it here too?
> >
> > something like cc::IsMemMovable? We could specialize on is_enum to avoid
> > having to name them all for instance..
> >
> > >
> > >
> > > >
> > > > >
> > > > > This came up when we were thinking of adding a vector of frames to
> > > > > PaintImage,
> > > > > which would violate the assumptions you've stated in
> paint_op_buffer.
> > > The
> > > > > author
> > > > > wasn't aware of these requirements, and honestly I wouldn't have
> been
> > > if it
> > > > > wasn't for a comment in PaintImage. In other words, without the
> > > comment in
> > > > > PaintImage, there is nothing that would raise a red flag for
> either the
> > > > > author
> > > > > or the reviewer. So, that comment is good. SafePaintOpData wouldn't
> > > have
> > > > > precluded the change though, since (1) it's in a seemingly
> unrelated
> > > file,
> > > > > (2)
> > > > > it would pass since it has no checks, and as a result (3) would
> > > provide a
> > > > > false
> > > > > sense of security since it looks like we're checking something
> with it,
> > > > > and it
> > > > > compiles, so looks like the change is good.
> > > > >
> > > >
> > > > These checks as is only protect against adding things to PaintOp
> > > directly,
> > > > which I think we do want. We could call something on nested types
> that we
> > > > control, and have them recursively do the same sort of thing.
> > >
> > > For one I'm not sure how much we'd be adding to this. That is, a clip
> rect
> > > will
> > > always have a rect kind of thing... Maybe you have some thoughts on
> other
> > > types
> > > that would eventually be here.
> > >
> >
> > Any data structure with a pointer in it could have this problem. Right
> now
> > they are all behind sk_sp<>s but in the future we will unwrap these to
> make
> > the stuff shared-memory-able. As we do so we need to look at the
> internals
> > of each type, and this is all a reminder to do so. Currently that's all
> > wrapped up inside PaintFlags but we will unwrap that over time and move
> > things out to the ops that use it, and drop refcounting for stuff. I can
> > imagine intermediate states that have pointers and then we crash a lot
> with
> > confusing stacks.
> >
> > My goal is mostly to give reviewers (and authors) lots of context to make
> > it harder to mess this up.
>
> I understand and share this goal. What I want in addition is to ensure
> that the
> author and the reviewer understand why we have this and realize that if
> the code
> compiles it doesn't mean that they haven't messed up. The patch as is
> makes it
> feel like as long as you can successfully compile, it's good.
>

What do you think of how we could enforce the IsMemMovable for nested
complex types we control? Do you like the idea of moving the template out
and using it (we could add a specialization on T::kIsMemMovable then, to
make it handle the recursion, with types declaring that along with using
the template on their members. Then we wouldn't add an specialization for
those types specifically, as they'd pass due to having the member.


>
>
> >
> >
> > > This also looks like at best a hope that a person
> > > adding something will notice most neighboring members are wrapped...
> and
> > > then
> > > all they really have to do is add another specialization that doesn't
> do
> > > anything.
> > >
> >
> > Yep, I depend on cargo culting and reviewers reading the file. I don't
> have
> > a better proposition tho.
> >
> >
> > > I guess what I'm saying is that unless the reviewer is one of the
> > > people that is already aware of these requirements, there are very few
> > > guards to
> > > hint at the requirements (save for maybe the comments).
> > >
> >
> > I am hoping that there is enough context for a reviewer who is not aware
> to
> > become aware when looking at a patch here. Do you think that's not the
> > case? Doing nothing would require them to be already aware. A comment
> would
> > give them a place to find it if they happened to. The templates will
> force
> > someone to write a new specialization beside a bunch of others that
> explain
> > things. At that point the reviewer gets a lot more to be like "hmm"
> about,
> > which is the goal.
> >
> >
> > > Maybe at least don't specialize every type that we have, and have a
> default
> > > implementation that does a static assert (with a message). That will
> > > trigger an
> > > error of a new non trivially copyable type that will explain what to
> do.
> > >
> >
> > How do I not specialize each type, but still get an error for an unknown
> > type?
>
> I'm not sure why you want an error if the type is trivially copyable. That
> seems
> like just extra effort on the author's part. I'm proposing a default
> implementation that static asserts for this condition, so any previously
> unknown
> type will be checked and only fail if it's not trivially copyable. The
> failure
> should provide a link to some sort of documentation. The documentation, in
> turn,
> should explain why we require this condition and what criteria has to be
> met
> before the author writes an exception specialization (like for PaintImage
> and
>
PaintFlags).
>

Oh I see what you mean. I thought you meant a static_assert that would
always fire in the default template. Done. Also used SafePaintOpData for
the primitive types.



>
>
> >
> >
> > > Can you also explain why we have this requirement (near the block
> where you
> > > state that we do have this requirement).
> > >
> >
> > // It is a requirement for every field in a PaintOp that we can memcpy it
> > to
> > // another place in memory and free the only one (without calling any
> >
> > // constructors or destructors).
> >
> > Is the comment on PaintOp. I will copy that roughly up to the template.
> >
> >
> > > As it stands now, it's not clear what
> > > the use case would be where the requirement is important.
> > >
> >
> > Do you think that comment bit is not clear or just its position?
>
> The comment position is fine. I'm only saying that the comment is stating
> that
> we have an assumption, but doesn't seem to provide a reason why we need
> that
> assumption. I just want a "why" blurb not just "what".
>

Ah, explaining that we realloc the buffer when it grows, gotcha. Done.


>
>
> >
> >
> > > Furthermore, I think
> > > it's important to note that simply memcpy-ing a ref counted pointer is
> not
> > > enough, the original copy _must_ be deleted without destructors. You
> > > allude a
> > > little bit about this in the comment, but I feel like it could use a
> bit
> > > more
> > > WARNING type of things around.
> > >
> >
> > // sk_sp<T> pointers behave if their memory address is changed
> >
> > Is the comment I have for sk_sp to explain that we're moving the address
> of
> > the object (memcpy+free) not memcpying. I can add some WARNING but I'm
> not
> > sure what to put. WARNING that we will free not just memcpy? That seems
> to
> > be strictly better though, as it's a moving address instead of skipping a
> > constructor but having an object suddenly, so warning about that feels
> > weird. Or would you like some WARNING in the PaintOp comment where I
> > explain the whole thing? I'll throw a WARNING at the top of that and see
> > what you think.
> >
>
> I think if you add the "why" above it might become more clear. I want it
> to be
> very clear that you can't just memcpy the whole thing and then use both
> copies,
> since you're going to double free ref counted pointers.
>

Ok I've added the way that we realloc the buffer. LMK if that's looking
better.


>
>
> >
> > >
> > >
> > > >
> > > > For skia-controlled types we have to just kinda hope really, it's at
> > > best a
> > > > breadcrumb trail if things go terribly wrong. But since they use
> sk_sp
> > > > liberally it's probably fine.
> > > >
> > >
> > >
> > > https://codereview.chromium.org/2899483002/
> > >
> >
> > --
> > 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 mailto:chromium-reviews+unsubscribe@chromium.org.
>
>
>
> https://codereview.chromium.org/2899483002/
>

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

[danakj, 2017-06-01 21:43:55]

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

[danakj, 2017-06-01 21:44:20]

I've uploaded those changes so far.

On Thu, Jun 1, 2017 at 5:43 PM, <danakj+owner@chromium.org> wrote:

> On Thu, Jun 1, 2017 at 5:26 PM, <vmpstr@chromium.org> wrote:
>
>> On 2017/06/01 21:08:40, danakj wrote:
>>
>> > On Thu, Jun 1, 2017 at 4:39 PM, <mailto:vmpstr@chromium.org> wrote:
>> >
>> > > >
>> > > > You're right that this doesn't guard the internals of the types, and
>> > > > there's 2 ways I can think of to address that.
>> > > > 1) Comments as I've done for PaintImage.
>> > > > 2) Force types to expose something, and that thing can check the
>> ok-ness
>> > > of
>> > > > its members. This moves things down one more level.
>> > > >
>> > > > Would 2 do better by you?
>> > > >
>> > >
>> > > Since these are compile-time checks, you can just add static asserts
>> for
>> > > each
>> > > member of the corresponding types. Ie,
>> > >
>> > > class PaintFoo {
>> > > ...
>> > > private:
>> > > static_assert(base::is_trivially_copyable<Bar>::value, "");
>> > > Bar bar_;
>> > >
>> > > // base::is_trivially_copyable<sk_sp<Baz>>::value won't pass, but
>> it's ok,
>> > > because ...
>> > > sk_sp<Baz> baz_;
>> > > };
>> > >
>> > > That looks like it's a better protection than either 1 or 2 (I do
>> want 1
>> > > though).
>> > >
>> >
>> > This is the kinda verbosity we avoided in the PaintOps. I don't mind
>> doing
>> > this, but I wonder if we should move the template up and use it here
>> too?
>> >
>> > something like cc::IsMemMovable? We could specialize on is_enum to avoid
>> > having to name them all for instance..
>> >
>> > >
>> > >
>> > > >
>> > > > >
>> > > > > This came up when we were thinking of adding a vector of frames to
>> > > > > PaintImage,
>> > > > > which would violate the assumptions you've stated in
>> paint_op_buffer.
>> > > The
>> > > > > author
>> > > > > wasn't aware of these requirements, and honestly I wouldn't have
>> been
>> > > if it
>> > > > > wasn't for a comment in PaintImage. In other words, without the
>> > > comment in
>> > > > > PaintImage, there is nothing that would raise a red flag for
>> either the
>> > > > > author
>> > > > > or the reviewer. So, that comment is good. SafePaintOpData
>> wouldn't
>> > > have
>> > > > > precluded the change though, since (1) it's in a seemingly
>> unrelated
>> > > file,
>> > > > > (2)
>> > > > > it would pass since it has no checks, and as a result (3) would
>> > > provide a
>> > > > > false
>> > > > > sense of security since it looks like we're checking something
>> with it,
>> > > > > and it
>> > > > > compiles, so looks like the change is good.
>> > > > >
>> > > >
>> > > > These checks as is only protect against adding things to PaintOp
>> > > directly,
>> > > > which I think we do want. We could call something on nested types
>> that we
>> > > > control, and have them recursively do the same sort of thing.
>> > >
>> > > For one I'm not sure how much we'd be adding to this. That is, a clip
>> rect
>> > > will
>> > > always have a rect kind of thing... Maybe you have some thoughts on
>> other
>> > > types
>> > > that would eventually be here.
>> > >
>> >
>> > Any data structure with a pointer in it could have this problem. Right
>> now
>> > they are all behind sk_sp<>s but in the future we will unwrap these to
>> make
>> > the stuff shared-memory-able. As we do so we need to look at the
>> internals
>> > of each type, and this is all a reminder to do so. Currently that's all
>> > wrapped up inside PaintFlags but we will unwrap that over time and move
>> > things out to the ops that use it, and drop refcounting for stuff. I can
>> > imagine intermediate states that have pointers and then we crash a lot
>> with
>> > confusing stacks.
>> >
>> > My goal is mostly to give reviewers (and authors) lots of context to
>> make
>> > it harder to mess this up.
>>
>> I understand and share this goal. What I want in addition is to ensure
>> that the
>> author and the reviewer understand why we have this and realize that if
>> the code
>> compiles it doesn't mean that they haven't messed up. The patch as is
>> makes it
>> feel like as long as you can successfully compile, it's good.
>>
>
> What do you think of how we could enforce the IsMemMovable for nested
> complex types we control? Do you like the idea of moving the template out
> and using it (we could add a specialization on T::kIsMemMovable then, to
> make it handle the recursion, with types declaring that along with using
> the template on their members. Then we wouldn't add an specialization for
> those types specifically, as they'd pass due to having the member.
>
>
>>
>>
>> >
>> >
>> > > This also looks like at best a hope that a person
>> > > adding something will notice most neighboring members are wrapped...
>> and
>> > > then
>> > > all they really have to do is add another specialization that doesn't
>> do
>> > > anything.
>> > >
>> >
>> > Yep, I depend on cargo culting and reviewers reading the file. I don't
>> have
>> > a better proposition tho.
>> >
>> >
>> > > I guess what I'm saying is that unless the reviewer is one of the
>> > > people that is already aware of these requirements, there are very few
>> > > guards to
>> > > hint at the requirements (save for maybe the comments).
>> > >
>> >
>> > I am hoping that there is enough context for a reviewer who is not
>> aware to
>> > become aware when looking at a patch here. Do you think that's not the
>> > case? Doing nothing would require them to be already aware. A comment
>> would
>> > give them a place to find it if they happened to. The templates will
>> force
>> > someone to write a new specialization beside a bunch of others that
>> explain
>> > things. At that point the reviewer gets a lot more to be like "hmm"
>> about,
>> > which is the goal.
>> >
>> >
>> > > Maybe at least don't specialize every type that we have, and have a
>> default
>> > > implementation that does a static assert (with a message). That will
>> > > trigger an
>> > > error of a new non trivially copyable type that will explain what to
>> do.
>> > >
>> >
>> > How do I not specialize each type, but still get an error for an unknown
>> > type?
>>
>> I'm not sure why you want an error if the type is trivially copyable.
>> That seems
>> like just extra effort on the author's part. I'm proposing a default
>> implementation that static asserts for this condition, so any previously
>> unknown
>> type will be checked and only fail if it's not trivially copyable. The
>> failure
>> should provide a link to some sort of documentation. The documentation,
>> in turn,
>> should explain why we require this condition and what criteria has to be
>> met
>> before the author writes an exception specialization (like for PaintImage
>> and
>>
> PaintFlags).
>>
>
> Oh I see what you mean. I thought you meant a static_assert that would
> always fire in the default template. Done. Also used SafePaintOpData for
> the primitive types.
>
>
>
>>
>>
>> >
>> >
>> > > Can you also explain why we have this requirement (near the block
>> where you
>> > > state that we do have this requirement).
>> > >
>> >
>> > // It is a requirement for every field in a PaintOp that we can memcpy
>> it
>> > to
>> > // another place in memory and free the only one (without calling any
>> >
>> > // constructors or destructors).
>> >
>> > Is the comment on PaintOp. I will copy that roughly up to the template.
>> >
>> >
>> > > As it stands now, it's not clear what
>> > > the use case would be where the requirement is important.
>> > >
>> >
>> > Do you think that comment bit is not clear or just its position?
>>
>> The comment position is fine. I'm only saying that the comment is stating
>> that
>> we have an assumption, but doesn't seem to provide a reason why we need
>> that
>> assumption. I just want a "why" blurb not just "what".
>>
>
> Ah, explaining that we realloc the buffer when it grows, gotcha. Done.
>
>
>>
>>
>> >
>> >
>> > > Furthermore, I think
>> > > it's important to note that simply memcpy-ing a ref counted pointer
>> is not
>> > > enough, the original copy _must_ be deleted without destructors. You
>> > > allude a
>> > > little bit about this in the comment, but I feel like it could use a
>> bit
>> > > more
>> > > WARNING type of things around.
>> > >
>> >
>> > // sk_sp<T> pointers behave if their memory address is changed
>> >
>> > Is the comment I have for sk_sp to explain that we're moving the
>> address of
>> > the object (memcpy+free) not memcpying. I can add some WARNING but I'm
>> not
>> > sure what to put. WARNING that we will free not just memcpy? That seems
>> to
>> > be strictly better though, as it's a moving address instead of skipping
>> a
>> > constructor but having an object suddenly, so warning about that feels
>> > weird. Or would you like some WARNING in the PaintOp comment where I
>> > explain the whole thing? I'll throw a WARNING at the top of that and see
>> > what you think.
>> >
>>
>> I think if you add the "why" above it might become more clear. I want it
>> to be
>> very clear that you can't just memcpy the whole thing and then use both
>> copies,
>> since you're going to double free ref counted pointers.
>>
>
> Ok I've added the way that we realloc the buffer. LMK if that's looking
> better.
>
>
>>
>>
>> >
>> > >
>> > >
>> > > >
>> > > > For skia-controlled types we have to just kinda hope really, it's at
>> > > best a
>> > > > breadcrumb trail if things go terribly wrong. But since they use
>> sk_sp
>> > > > liberally it's probably fine.
>> > > >
>> > >
>> > >
>> > > https://codereview.chromium.org/2899483002/
>> > >
>> >
>> > --
>> > 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 mailto:chromium-reviews+unsubscribe@chromium.org.
>>
>>
>>
>> https://codereview.chromium.org/2899483002/
>>
>
>

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

[commit-bot: I haz the power, 2017-06-01 21:44:37]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[vmpstr, 2017-06-01 22:00:55]

On 2017/06/01 21:43:46, danakj wrote:
> On Thu, Jun 1, 2017 at 5:26 PM, <mailto:vmpstr@chromium.org> wrote:
> 
> > On 2017/06/01 21:08:40, danakj wrote:
> >
> > > On Thu, Jun 1, 2017 at 4:39 PM, <mailto:vmpstr@chromium.org> wrote:
> > >
> > > > >
> > > > > You're right that this doesn't guard the internals of the types, and
> > > > > there's 2 ways I can think of to address that.
> > > > > 1) Comments as I've done for PaintImage.
> > > > > 2) Force types to expose something, and that thing can check the
> > ok-ness
> > > > of
> > > > > its members. This moves things down one more level.
> > > > >
> > > > > Would 2 do better by you?
> > > > >
> > > >
> > > > Since these are compile-time checks, you can just add static asserts
> > for
> > > > each
> > > > member of the corresponding types. Ie,
> > > >
> > > > class PaintFoo {
> > > > ...
> > > > private:
> > > > static_assert(base::is_trivially_copyable<Bar>::value, "");
> > > > Bar bar_;
> > > >
> > > > // base::is_trivially_copyable<sk_sp<Baz>>::value won't pass, but
> > it's ok,
> > > > because ...
> > > > sk_sp<Baz> baz_;
> > > > };
> > > >
> > > > That looks like it's a better protection than either 1 or 2 (I do want
> > 1
> > > > though).
> > > >
> > >
> > > This is the kinda verbosity we avoided in the PaintOps. I don't mind
> > doing
> > > this, but I wonder if we should move the template up and use it here too?
> > >
> > > something like cc::IsMemMovable? We could specialize on is_enum to avoid
> > > having to name them all for instance..
> > >
> > > >
> > > >
> > > > >
> > > > > >
> > > > > > This came up when we were thinking of adding a vector of frames to
> > > > > > PaintImage,
> > > > > > which would violate the assumptions you've stated in
> > paint_op_buffer.
> > > > The
> > > > > > author
> > > > > > wasn't aware of these requirements, and honestly I wouldn't have
> > been
> > > > if it
> > > > > > wasn't for a comment in PaintImage. In other words, without the
> > > > comment in
> > > > > > PaintImage, there is nothing that would raise a red flag for
> > either the
> > > > > > author
> > > > > > or the reviewer. So, that comment is good. SafePaintOpData wouldn't
> > > > have
> > > > > > precluded the change though, since (1) it's in a seemingly
> > unrelated
> > > > file,
> > > > > > (2)
> > > > > > it would pass since it has no checks, and as a result (3) would
> > > > provide a
> > > > > > false
> > > > > > sense of security since it looks like we're checking something
> > with it,
> > > > > > and it
> > > > > > compiles, so looks like the change is good.
> > > > > >
> > > > >
> > > > > These checks as is only protect against adding things to PaintOp
> > > > directly,
> > > > > which I think we do want. We could call something on nested types
> > that we
> > > > > control, and have them recursively do the same sort of thing.
> > > >
> > > > For one I'm not sure how much we'd be adding to this. That is, a clip
> > rect
> > > > will
> > > > always have a rect kind of thing... Maybe you have some thoughts on
> > other
> > > > types
> > > > that would eventually be here.
> > > >
> > >
> > > Any data structure with a pointer in it could have this problem. Right
> > now
> > > they are all behind sk_sp<>s but in the future we will unwrap these to
> > make
> > > the stuff shared-memory-able. As we do so we need to look at the
> > internals
> > > of each type, and this is all a reminder to do so. Currently that's all
> > > wrapped up inside PaintFlags but we will unwrap that over time and move
> > > things out to the ops that use it, and drop refcounting for stuff. I can
> > > imagine intermediate states that have pointers and then we crash a lot
> > with
> > > confusing stacks.
> > >
> > > My goal is mostly to give reviewers (and authors) lots of context to make
> > > it harder to mess this up.
> >
> > I understand and share this goal. What I want in addition is to ensure
> > that the
> > author and the reviewer understand why we have this and realize that if
> > the code
> > compiles it doesn't mean that they haven't messed up. The patch as is
> > makes it
> > feel like as long as you can successfully compile, it's good.
> >
> 
> What do you think of how we could enforce the IsMemMovable for nested
> complex types we control? Do you like the idea of moving the template out
> and using it (we could add a specialization on T::kIsMemMovable then, to
> make it handle the recursion, with types declaring that along with using
> the template on their members. Then we wouldn't add an specialization for
> those types specifically, as they'd pass due to having the member.

Something along these lines works for me. It's essentially the same as a static
assert, but just wrapped into fewer lines.

> 
> 
> >
> >
> > >
> > >
> > > > This also looks like at best a hope that a person
> > > > adding something will notice most neighboring members are wrapped...
> > and
> > > > then
> > > > all they really have to do is add another specialization that doesn't
> > do
> > > > anything.
> > > >
> > >
> > > Yep, I depend on cargo culting and reviewers reading the file. I don't
> > have
> > > a better proposition tho.
> > >
> > >
> > > > I guess what I'm saying is that unless the reviewer is one of the
> > > > people that is already aware of these requirements, there are very few
> > > > guards to
> > > > hint at the requirements (save for maybe the comments).
> > > >
> > >
> > > I am hoping that there is enough context for a reviewer who is not aware
> > to
> > > become aware when looking at a patch here. Do you think that's not the
> > > case? Doing nothing would require them to be already aware. A comment
> > would
> > > give them a place to find it if they happened to. The templates will
> > force
> > > someone to write a new specialization beside a bunch of others that
> > explain
> > > things. At that point the reviewer gets a lot more to be like "hmm"
> > about,
> > > which is the goal.
> > >
> > >
> > > > Maybe at least don't specialize every type that we have, and have a
> > default
> > > > implementation that does a static assert (with a message). That will
> > > > trigger an
> > > > error of a new non trivially copyable type that will explain what to
> > do.
> > > >
> > >
> > > How do I not specialize each type, but still get an error for an unknown
> > > type?
> >
> > I'm not sure why you want an error if the type is trivially copyable. That
> > seems
> > like just extra effort on the author's part. I'm proposing a default
> > implementation that static asserts for this condition, so any previously
> > unknown
> > type will be checked and only fail if it's not trivially copyable. The
> > failure
> > should provide a link to some sort of documentation. The documentation, in
> > turn,
> > should explain why we require this condition and what criteria has to be
> > met
> > before the author writes an exception specialization (like for PaintImage
> > and
> >
> PaintFlags).
> >
> 
> Oh I see what you mean. I thought you meant a static_assert that would
> always fire in the default template. Done. Also used SafePaintOpData for
> the primitive types.
> 
> 
> 
> >
> >
> > >
> > >
> > > > Can you also explain why we have this requirement (near the block
> > where you
> > > > state that we do have this requirement).
> > > >
> > >
> > > // It is a requirement for every field in a PaintOp that we can memcpy it
> > > to
> > > // another place in memory and free the only one (without calling any
> > >
> > > // constructors or destructors).
> > >
> > > Is the comment on PaintOp. I will copy that roughly up to the template.
> > >
> > >
> > > > As it stands now, it's not clear what
> > > > the use case would be where the requirement is important.
> > > >
> > >
> > > Do you think that comment bit is not clear or just its position?
> >
> > The comment position is fine. I'm only saying that the comment is stating
> > that
> > we have an assumption, but doesn't seem to provide a reason why we need
> > that
> > assumption. I just want a "why" blurb not just "what".
> >
> 
> Ah, explaining that we realloc the buffer when it grows, gotcha. Done.
> 
> 
> >
> >
> > >
> > >
> > > > Furthermore, I think
> > > > it's important to note that simply memcpy-ing a ref counted pointer is
> > not
> > > > enough, the original copy _must_ be deleted without destructors. You
> > > > allude a
> > > > little bit about this in the comment, but I feel like it could use a
> > bit
> > > > more
> > > > WARNING type of things around.
> > > >
> > >
> > > // sk_sp<T> pointers behave if their memory address is changed
> > >
> > > Is the comment I have for sk_sp to explain that we're moving the address
> > of
> > > the object (memcpy+free) not memcpying. I can add some WARNING but I'm
> > not
> > > sure what to put. WARNING that we will free not just memcpy? That seems
> > to
> > > be strictly better though, as it's a moving address instead of skipping a
> > > constructor but having an object suddenly, so warning about that feels
> > > weird. Or would you like some WARNING in the PaintOp comment where I
> > > explain the whole thing? I'll throw a WARNING at the top of that and see
> > > what you think.
> > >
> >
> > I think if you add the "why" above it might become more clear. I want it
> > to be
> > very clear that you can't just memcpy the whole thing and then use both
> > copies,
> > since you're going to double free ref counted pointers.
> >
> 
> Ok I've added the way that we realloc the buffer. LMK if that's looking
> better.
> 

Yep that looks good. Thanks.

https://codereview.chromium.org/2899483002/diff/160001/cc/paint/paint_op_buff...
File cc/paint/paint_op_buffer.h (right):

https://codereview.chromium.org/2899483002/diff/160001/cc/paint/paint_op_buff...
cc/paint/paint_op_buffer.h:81: struct SafePaintOpData;
Don't need the forward declare anymore

https://codereview.chromium.org/2899483002/diff/160001/cc/paint/paint_op_buff...
cc/paint/paint_op_buffer.h:160: struct CC_PAINT_EXPORT PaintOp {
maybe we can add something like

template <typename T>
using Member = SafePaintOpData<T>::type;

to PaintOp and then everything can just be 

Member<bool> flag; type of things? AFAIK that would still trigger the static
assert?

(this is a nit, I'm just trying to improve the ergonomics of using this a bit,
but maybe this is more explicit. not sure)

[commit-bot: I haz the power, 2017-06-02 01:21:19]

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

[commit-bot: I haz the power, 2017-06-02 01:21:20]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)