Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(2)

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
4 days, 21 hours ago by danakj
Modified:
23 hours, 26 minutes ago
Reviewers:
enne
CC:
chromium-reviews, cc-bugs_chromium.org, vmpstr, weiliangc, yngve_vivaldi.com
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

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

Patch Set 1 #

Patch Set 2 : check-copy better-comment #

Patch Set 3 : check-copy better-better-comment #

Total comments: 1

Messages

Total messages: 16 (10 generated)
danakj
4 days, 21 hours ago (2017-05-19 20:31:05 UTC) #3
danakj
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#newcode58 cc/paint/paint_image.h:58: // Because PaintImage is stored in PaintOpBuffer it must ...
4 days, 21 hours ago (2017-05-19 20:35:35 UTC) #10
enne
I...have a lot of mixed feelings about this patch. I feel like the comments and ...
2 days ago (2017-05-22 17:19:45 UTC) #13
danakj
On Mon, May 22, 2017 at 1:19 PM, enne@chromium.org via codereview.chromium.org <reply@chromiumcodereview-hr.appspotmail.com> wrote: > I...have ...
1 day ago (2017-05-23 17:04:58 UTC) #14
enne
On 2017/05/23 at 17:04:58, danakj wrote: > I could move it all to the top. ...
23 hours, 33 minutes ago (2017-05-23 18:15:25 UTC) #15
danakj
23 hours, 26 minutes ago (2017-05-23 18:22:22 UTC) #16
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.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 650457f06