Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(380)

Issue 2632713003: Remove ~MaybeHandle and statically assert that handles are trivially copyable

Created:
3 years, 11 months ago by Reid Kleckner
Modified:
3 years, 9 months ago
CC:
v8-reviews_googlegroups.com
Target Ref:
refs/pending/heads/master
Project:
v8
Visibility:
Public.

Description

Remove ~MaybeHandle and statically assert that handles are trivially copyable While investigating http://crbug.com/457078#c79, I noticed that MaybeHandle has an unecessary user provided destructor. On most platforms, this means that MaybeHandle will be passed by hidden reference. Remove the destructor so that it can be passed in registers when possible, and statically assert that these objects are trivially copyable so this doesn't regress. R=jochen@chromium.org,machenbach@chromium.org BUG=chromium:457078

Patch Set 1 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+10 lines, -1 line) Patch
M src/handles.h View 1 chunk +0 lines, -1 line 0 comments Download
M src/handles.cc View 1 chunk +10 lines, -0 lines 0 comments Download

Messages

Total messages: 10 (3 generated)
Reid Kleckner
3 years, 11 months ago (2017-01-13 17:23:13 UTC) #1
jochen (gone - plz use gerrit)
Yang is a better reviewer for this
3 years, 11 months ago (2017-01-13 18:51:29 UTC) #3
Yang
On 2017/01/13 18:51:29, jochen wrote: > Yang is a better reviewer for this lgtm.
3 years, 11 months ago (2017-01-17 12:40:15 UTC) #4
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2632713003/1
3 years, 11 months ago (2017-01-17 19:31:02 UTC) #6
commit-bot: I haz the power
Try jobs failed on following builders: v8_linux64_avx2_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/builds/15302) v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED, ...
3 years, 11 months ago (2017-01-17 19:34:29 UTC) #8
agrieve
On 2017/01/17 19:34:29, commit-bot: I haz the power wrote: > Try jobs failed on following ...
3 years, 9 months ago (2017-02-28 03:31:41 UTC) #9
Reid Kleckner
3 years, 9 months ago (2017-03-01 22:14:50 UTC) #10
On 2017/02/28 03:31:41, agrieve wrote:
> On 2017/01/17 19:34:29, commit-bot: I haz the power wrote:
> > Try jobs failed on following builders:
> >   v8_linux64_avx2_rel_ng on master.tryserver.v8 (JOB_FAILED,
> >
>
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/buil...)
> >   v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED,
> >
>
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...)
> >   v8_linux_arm64_rel_ng on master.tryserver.v8 (JOB_FAILED,
> >
>
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel_ng/build...)
> >   v8_linux_gcc_compile_rel on master.tryserver.v8 (JOB_FAILED,
> >
>
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_gcc_compile_rel/bu...)
> 
> Wondering if this is still a valid change?

Probably, but it looks like V8's toolchains don't like static_assert. I haven't
had time to get back and fix this.

Powered by Google App Engine
This is Rietveld 408576698