Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
4 years, 2 months ago by rossberg Modified:
4 years, 2 months ago Reviewers:
ahaas CC:
v8-reviews_googlegroups.com Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
DescriptionImplement Table#length and Table#get
R=ahaas@chromium.org
BUG=
Committed: https://crrev.com/d95b754319ea84d339ff748680125f39574c1ab1
Cr-Commit-Position: refs/heads/master@{#40273}
Patch Set 1 #
Total comments: 10
Patch Set 2 : Comments; removed a redundant CHECK #Patch Set 3 : Git format nonsense #
Created: 4 years, 2 months ago
Messages
Total messages: 14 (5 generated)
https://codereview.chromium.org/2411963003/diff/1/src/wasm/wasm-js.cc File src/wasm/wasm-js.cc (right): https://codereview.chromium.org/2411963003/diff/1/src/wasm/wasm-js.cc#newcode491 src/wasm/wasm-js.cc:491: if (!BrandCheck(isolate, Utils::OpenHandle(*args.This()), you call Utils::OpenHandle(*args.This()) twice here. Would it be worth storing the result in a variable? https://codereview.chromium.org/2411963003/diff/1/src/wasm/wasm-js.cc#newcode499 src/wasm/wasm-js.cc:499: i::Handle<i::Object> array(receiver->GetInternalField(0), i_isolate); Can you use a constant here instead of '0'? https://codereview.chromium.org/2411963003/diff/1/src/wasm/wasm-js.cc#newcode529 src/wasm/wasm-js.cc:529: if (args.Length() < 1) { Does the spec actually say that a TypeError should be thrown if args.Length() < 1? https://codereview.chromium.org/2411963003/diff/1/src/wasm/wasm-js.cc#newcode539 src/wasm/wasm-js.cc:539: i::Handle<i::Object> array(receiver->GetInternalField(0), i_isolate); same here https://codereview.chromium.org/2411963003/diff/1/src/wasm/wasm-js.cc#newcode559 src/wasm/wasm-js.cc:559: if (args.Length() < 1) { This if is not needed, the next if also takes care of it.
https://codereview.chromium.org/2411963003/diff/1/src/wasm/wasm-js.cc File src/wasm/wasm-js.cc (right): https://codereview.chromium.org/2411963003/diff/1/src/wasm/wasm-js.cc#newcode491 src/wasm/wasm-js.cc:491: if (!BrandCheck(isolate, Utils::OpenHandle(*args.This()), On 2016/10/13 09:12:58, ahaas wrote: > you call Utils::OpenHandle(*args.This()) twice here. Would it be worth storing > the result in a variable? It's just an accessor, so should be fast enough. https://codereview.chromium.org/2411963003/diff/1/src/wasm/wasm-js.cc#newcode499 src/wasm/wasm-js.cc:499: i::Handle<i::Object> array(receiver->GetInternalField(0), i_isolate); On 2016/10/13 09:12:59, ahaas wrote: > Can you use a constant here instead of '0'? Done (also for some other field indices). https://codereview.chromium.org/2411963003/diff/1/src/wasm/wasm-js.cc#newcode529 src/wasm/wasm-js.cc:529: if (args.Length() < 1) { On 2016/10/13 09:12:58, ahaas wrote: > Does the spec actually say that a TypeError should be thrown if args.Length() < > 1? Changed. https://codereview.chromium.org/2411963003/diff/1/src/wasm/wasm-js.cc#newcode539 src/wasm/wasm-js.cc:539: i::Handle<i::Object> array(receiver->GetInternalField(0), i_isolate); On 2016/10/13 09:12:58, ahaas wrote: > same here Done. https://codereview.chromium.org/2411963003/diff/1/src/wasm/wasm-js.cc#newcode559 src/wasm/wasm-js.cc:559: if (args.Length() < 1) { On 2016/10/13 09:12:59, ahaas wrote: > This if is not needed, the next if also takes care of it. Obsolete.
lgtm
The CQ bit was checked by rossberg@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: v8_presubmit on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/26394)
The CQ bit was checked by rossberg@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ahaas@chromium.org Link to the patchset: https://codereview.chromium.org/2411963003/#ps40001 (title: "Git format nonsense")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Implement Table#length and Table#get R=ahaas@chromium.org BUG= ========== to ========== Implement Table#length and Table#get R=ahaas@chromium.org BUG= Committed: https://crrev.com/d95b754319ea84d339ff748680125f39574c1ab1 Cr-Commit-Position: refs/heads/master@{#40273} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/d95b754319ea84d339ff748680125f39574c1ab1 Cr-Commit-Position: refs/heads/master@{#40273} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
