Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1961453002:
[wasm] Clean up test case (Closed)
|
Created:
4 years, 7 months ago by Clemens Hammacher Modified:
4 years, 7 months ago CC:
v8-reviews_googlegroups.com Base URL:
https://chromium.googlesource.com/v8/v8.git@wasm-offset-table-4 Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
Description[wasm] Clean up test case
R=titzer@chromium.org, yangguo@chromium.org
Committed: https://crrev.com/452b7f2483a54d781a2b1ba74516d91e035a85ed
Cr-Commit-Position: refs/heads/master@{#36354}
Patch Set 1 #Patch Set 2 : update expected test outcome #
Total comments: 3
Patch Set 3 : rebase on larger CL which already includes most of the changes #Patch Set 4 : rebase #Messages
Total messages: 18 (7 generated)
Description was changed from ========== [wasm] Return function name "<WASM>" for unnamed function R=titzer@chromium.org, yangguo@chromium.org ========== to ========== [wasm] Return function name "<WASM>" for unnamed functions R=titzer@chromium.org, yangguo@chromium.org ==========
On 2016/05/06 08:39:51, Clemens Hammacher wrote: lgtm
https://codereview.chromium.org/1961453002/diff/20001/src/js/messages.js File src/js/messages.js (right): https://codereview.chromium.org/1961453002/diff/20001/src/js/messages.js#newc... src/js/messages.js:638: return func_name ? func_name : "<WASM>"; Can we do a IS_UNDEFINED check if we only expect undefined or strings?
clemensh@google.com changed reviewers: + clemensh@google.com
https://codereview.chromium.org/1961453002/diff/20001/src/js/messages.js File src/js/messages.js (right): https://codereview.chromium.org/1961453002/diff/20001/src/js/messages.js#newc... src/js/messages.js:638: return func_name ? func_name : "<WASM>"; On 2016/05/09 05:34:55, Yang wrote: > Can we do a IS_UNDEFINED check if we only expect undefined or strings? Unfortunately not. At least not without further changes in the way function names are stored inside the wasm modules. It does not distinguish currently between "no name" and "empty name". So we always get an empty string if no name was set, and only get undefined if e.g. the string is no valid UTF-8.
https://codereview.chromium.org/1961453002/diff/20001/src/js/messages.js File src/js/messages.js (right): https://codereview.chromium.org/1961453002/diff/20001/src/js/messages.js#newc... src/js/messages.js:638: return func_name ? func_name : "<WASM>"; On 2016/05/09 07:38:46, clemensh wrote: > On 2016/05/09 05:34:55, Yang wrote: > > Can we do a IS_UNDEFINED check if we only expect undefined or strings? > > Unfortunately not. At least not without further changes in the way function > names are stored inside the wasm modules. It does not distinguish currently > between "no name" and "empty name". So we always get an empty string if no name > was set, and only get undefined if e.g. the string is no valid UTF-8. I see. Can we hide this logic inside %WasmGetFunctionName? I'd like to avoid implicit conversions, and it should be the job of the wasm part of V8 to deliver the appropriate name.
On 2016/05/11 05:49:41, Yang wrote: > https://codereview.chromium.org/1961453002/diff/20001/src/js/messages.js > File src/js/messages.js (right): > > https://codereview.chromium.org/1961453002/diff/20001/src/js/messages.js#newc... > src/js/messages.js:638: return func_name ? func_name : "<WASM>"; > I see. Can we hide this logic inside %WasmGetFunctionName? I'd like to avoid > implicit conversions, and it should be the job of the wasm part of V8 to deliver > the appropriate name. I now thought a bit more about it and defined the precise behaviour in the Design Doc. This patch is now rebased on another one which changes larger parts. What remains here is just the cleanup of a test case. We cannot completely hide the logic inside %WasmGetFunctionName, since the string representation differs in some cases from the information returned by CallSite.getFunctionName().
Description was changed from ========== [wasm] Return function name "<WASM>" for unnamed functions R=titzer@chromium.org, yangguo@chromium.org ========== to ========== [wasm] Clean up test case R=titzer@chromium.org, yangguo@chromium.org ==========
lgtm
On 2016/05/12 13:03:24, titzer wrote: > lgtm lgtm.
The CQ bit was checked by clemensh@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from titzer@chromium.org, yangguo@chromium.org Link to the patchset: https://codereview.chromium.org/1961453002/#ps60001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1961453002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1961453002/60001
Message was sent while issue was closed.
Description was changed from ========== [wasm] Clean up test case R=titzer@chromium.org, yangguo@chromium.org ========== to ========== [wasm] Clean up test case R=titzer@chromium.org, yangguo@chromium.org ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== [wasm] Clean up test case R=titzer@chromium.org, yangguo@chromium.org ========== to ========== [wasm] Clean up test case R=titzer@chromium.org, yangguo@chromium.org Committed: https://crrev.com/452b7f2483a54d781a2b1ba74516d91e035a85ed Cr-Commit-Position: refs/heads/master@{#36354} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/452b7f2483a54d781a2b1ba74516d91e035a85ed Cr-Commit-Position: refs/heads/master@{#36354} |
