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

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 1196193014: Do not add extra argument for new.target (Closed)

Created:
5 years, 6 months ago by Dmitry Lomov (no reviews)
Modified:
5 years, 6 months ago
CC:
v8-dev
Base URL:
https://chromium.googlesource.com/v8/v8.git@master
Target Ref:
refs/pending/heads/master
Project:
v8
Visibility:
Public.

Description

Do not add extra argument for new.target JSConstructStub for subclass constructors instead locates new.target in a known location on the stack. R=arv@chromium.org,adamk@chromium.org BUG=v8:3886 LOG=N Committed: https://crrev.com/8196c28a94f62dec026f2b564ba81d690a4ed593 Cr-Commit-Position: refs/heads/master@{#29238}

Patch Set 1 #

Total comments: 4

Patch Set 2 : ports #

Total comments: 2

Patch Set 3 : Comment added #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+170 lines, -244 lines) Patch
M src/arm/builtins-arm.cc View 1 2 2 chunks +2 lines, -3 lines 0 comments Download
M src/arm/code-stubs-arm.cc View 1 4 chunks +0 lines, -13 lines 0 comments Download
M src/arm/full-codegen-arm.cc View 1 6 chunks +21 lines, -22 lines 2 comments Download
M src/arm64/builtins-arm64.cc View 1 2 3 chunks +2 lines, -5 lines 0 comments Download
M src/arm64/code-stubs-arm64.cc View 1 4 chunks +0 lines, -14 lines 0 comments Download
M src/arm64/full-codegen-arm64.cc View 1 5 chunks +24 lines, -18 lines 0 comments Download
M src/code-stubs.h View 3 chunks +2 lines, -11 lines 0 comments Download
M src/ia32/builtins-ia32.cc View 1 2 2 chunks +2 lines, -4 lines 0 comments Download
M src/ia32/code-stubs-ia32.cc View 4 chunks +0 lines, -17 lines 0 comments Download
M src/ia32/full-codegen-ia32.cc View 1 6 chunks +26 lines, -18 lines 0 comments Download
M src/mips/builtins-mips.cc View 1 2 2 chunks +3 lines, -4 lines 0 comments Download
M src/mips/code-stubs-mips.cc View 1 4 chunks +0 lines, -12 lines 0 comments Download
M src/mips/full-codegen-mips.cc View 1 5 chunks +28 lines, -19 lines 0 comments Download
M src/mips64/builtins-mips64.cc View 1 2 2 chunks +3 lines, -4 lines 0 comments Download
M src/mips64/code-stubs-mips64.cc View 1 4 chunks +0 lines, -14 lines 0 comments Download
M src/mips64/full-codegen-mips64.cc View 1 5 chunks +27 lines, -19 lines 0 comments Download
M src/objects.cc View 1 chunk +1 line, -6 lines 0 comments Download
M src/x64/builtins-x64.cc View 1 2 2 chunks +3 lines, -4 lines 0 comments Download
M src/x64/code-stubs-x64.cc View 1 4 chunks +0 lines, -18 lines 0 comments Download
M src/x64/full-codegen-x64.cc View 1 5 chunks +26 lines, -19 lines 0 comments Download

Messages

Total messages: 17 (4 generated)
Dmitry Lomov (no reviews)
PTAL, ia32 only for now.
5 years, 6 months ago (2015-06-23 12:44:45 UTC) #1
arv (Not doing code reviews)
Thanks. Does this pass all the tests on ia32? https://codereview.chromium.org/1196193014/diff/1/src/ia32/full-codegen-ia32.cc File src/ia32/full-codegen-ia32.cc (right): https://codereview.chromium.org/1196193014/diff/1/src/ia32/full-codegen-ia32.cc#newcode269 src/ia32/full-codegen-ia32.cc:269: ...
5 years, 6 months ago (2015-06-23 14:13:46 UTC) #2
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1196193014/20001
5 years, 6 months ago (2015-06-23 15:06:23 UTC) #4
Dmitry Lomov (no reviews)
Yup, passes all tests https://codereview.chromium.org/1196193014/diff/1/src/ia32/full-codegen-ia32.cc File src/ia32/full-codegen-ia32.cc (right): https://codereview.chromium.org/1196193014/diff/1/src/ia32/full-codegen-ia32.cc#newcode269 src/ia32/full-codegen-ia32.cc:269: SetVar(new_target_var, eax, ebx, edx); On ...
5 years, 6 months ago (2015-06-23 15:06:25 UTC) #5
adamk
https://codereview.chromium.org/1196193014/diff/1/src/ia32/builtins-ia32.cc File src/ia32/builtins-ia32.cc (right): https://codereview.chromium.org/1196193014/diff/1/src/ia32/builtins-ia32.cc#newcode556 src/ia32/builtins-ia32.cc:556: __ mov(ebx, Operand(esp, kPointerSize)); This could use a comment, ...
5 years, 6 months ago (2015-06-23 15:13:38 UTC) #6
commit-bot: I haz the power
Dry run: This issue passed the CQ dry run.
5 years, 6 months ago (2015-06-23 15:33:45 UTC) #8
arv (Not doing code reviews)
LGTM https://codereview.chromium.org/1196193014/diff/20001/src/arm/builtins-arm.cc File src/arm/builtins-arm.cc (right): https://codereview.chromium.org/1196193014/diff/20001/src/arm/builtins-arm.cc#newcode813 src/arm/builtins-arm.cc:813: __ ldr(r1, MemOperand(sp, kPointerSize)); Add comment about this ...
5 years, 6 months ago (2015-06-23 15:52:59 UTC) #9
Dmitry Lomov (no reviews)
Comments addressed, landing. https://codereview.chromium.org/1196193014/diff/1/src/ia32/builtins-ia32.cc File src/ia32/builtins-ia32.cc (right): https://codereview.chromium.org/1196193014/diff/1/src/ia32/builtins-ia32.cc#newcode556 src/ia32/builtins-ia32.cc:556: __ mov(ebx, Operand(esp, kPointerSize)); On 2015/06/23 ...
5 years, 6 months ago (2015-06-23 16:18:51 UTC) #10
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1196193014/40001
5 years, 6 months ago (2015-06-23 16:19:24 UTC) #13
commit-bot: I haz the power
Committed patchset #3 (id:40001)
5 years, 6 months ago (2015-06-23 16:50:47 UTC) #14
commit-bot: I haz the power
Patchset 3 (id:??) landed as https://crrev.com/8196c28a94f62dec026f2b564ba81d690a4ed593 Cr-Commit-Position: refs/heads/master@{#29238}
5 years, 6 months ago (2015-06-23 16:51:08 UTC) #15
arv (Not doing code reviews)
https://codereview.chromium.org/1196193014/diff/40001/src/arm/full-codegen-arm.cc File src/arm/full-codegen-arm.cc (right): https://codereview.chromium.org/1196193014/diff/40001/src/arm/full-codegen-arm.cc#newcode255 src/arm/full-codegen-arm.cc:255: __ cmp(r1, Operand(Smi::FromInt(StackFrame::ARGUMENTS_ADAPTOR))); You missed a branch here.
5 years, 6 months ago (2015-06-26 00:47:41 UTC) #16
arv (Not doing code reviews)
5 years, 6 months ago (2015-06-26 15:15:34 UTC) #17
Message was sent while issue was closed. 
https://codereview.chromium.org/1196193014/diff/40001/src/arm/full-codegen-ar...
File src/arm/full-codegen-arm.cc (right):

https://codereview.chromium.org/1196193014/diff/40001/src/arm/full-codegen-ar...
src/arm/full-codegen-arm.cc:255: __ cmp(r1,
Operand(Smi::FromInt(StackFrame::ARGUMENTS_ADAPTOR)));
On 2015/06/26 00:47:41, arv wrote:
> You missed a branch here.

Never mind... I see the trailing eq now... Something else is going wrong in my
case.

Powered by Google App Engine
This is Rietveld 408576698