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

Issue 2503313002: [wasm] Fix imports with numbers as name (Closed)

Created:
4 years, 1 month ago by Clemens Hammacher
Modified:
4 years, 1 month ago
Reviewers:
titzer, ahaas
CC:
v8-reviews_googlegroups.com
Target Ref:
refs/pending/heads/master
Project:
v8
Visibility:
Public.

Description

[wasm] Fix imports with numbers as name Object::GetProperty fails if the given name is a valid array index. This CL switches to Object::GetPropertyOrElement for lookups of imports. The new tests check that we now accept numbers as module name or function name in FFI. R=ahaas@chromium.org, titzer@chromium.org Committed: https://crrev.com/c2db3b38989feb2825c7ce15fd944f446c6a2152 Cr-Commit-Position: refs/heads/master@{#41022}

Patch Set 1 #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+31 lines, -6 lines) Patch
M src/wasm/wasm-module.cc View 2 chunks +4 lines, -2 lines 0 comments Download
M test/mjsunit/wasm/ffi.js View 2 chunks +27 lines, -4 lines 2 comments Download

Messages

Total messages: 12 (6 generated)
Clemens Hammacher
4 years, 1 month ago (2016-11-16 10:24:09 UTC) #3
titzer
On 2016/11/16 10:24:09, Clemens Hammacher wrote: lgtm
4 years, 1 month ago (2016-11-16 10:26:51 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/2503313002/1
4 years, 1 month ago (2016-11-16 10:33:40 UTC) #8
commit-bot: I haz the power
Committed patchset #1 (id:1)
4 years, 1 month ago (2016-11-16 10:36:45 UTC) #9
ahaas
lgtm with nits https://codereview.chromium.org/2503313002/diff/1/test/mjsunit/wasm/ffi.js File test/mjsunit/wasm/ffi.js (right): https://codereview.chromium.org/2503313002/diff/1/test/mjsunit/wasm/ffi.js#newcode298 test/mjsunit/wasm/ffi.js:298: (function testCallPrint() { All other tests ...
4 years, 1 month ago (2016-11-16 10:51:18 UTC) #10
commit-bot: I haz the power
4 years, 1 month ago (2016-11-17 22:35:39 UTC) #12
Message was sent while issue was closed.
Patchset 1 (id:??) landed as
https://crrev.com/c2db3b38989feb2825c7ce15fd944f446c6a2152
Cr-Commit-Position: refs/heads/master@{#41022}

Powered by Google App Engine
This is Rietveld 408576698