Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1839843002:
wasm: implemente WasmFrame::cast, fix inheritance (Closed)
Descriptionwasm: implemente WasmFrame::cast, fix inheritance
wasm_to_js and js_to_wasm both derive from wasm, which was confusing because is_wasm wasn't true for them and that made WasmFrame::cast awkward. Make them derive from StubFrame instead.
R=bradnelson@chromium.org, titzer@chromium.org
Committed: https://crrev.com/18b44702126d9b27f954f7c9b7168716369b0478
Cr-Commit-Position: refs/heads/master@{#35115}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Fix inheritance #
Total comments: 2
Patch Set 3 : Fix leftover bad change #
Messages
Total messages: 28 (10 generated)
The CQ bit was checked by jfb@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1839843002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1839843002/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
bradnelson@google.com changed reviewers: + bradnelson@google.com
https://codereview.chromium.org/1839843002/diff/1/src/frames.h File src/frames.h (right): https://codereview.chromium.org/1839843002/diff/1/src/frames.h#newcode417 src/frames.h:417: bool is_wasm() const { Keeping the 3 sorts of wasm frames orthogonal seems clearer to me. https://codereview.chromium.org/1839843002/diff/1/src/frames.h#newcode967 src/frames.h:967: DCHECK(frame->is_wasm()); Why not preserve the existing categories and dcheck or of all 3 here?
Or how about rename the internal frames, WASM_INTERNAL and call the combined set Wasm?
LMK which way you'd rather go. The current code is different from the pre-existing code, and I already thought these frames were confusing :-) https://codereview.chromium.org/1839843002/diff/1/src/frames.h File src/frames.h (right): https://codereview.chromium.org/1839843002/diff/1/src/frames.h#newcode417 src/frames.h:417: bool is_wasm() const { On 2016/03/28 23:46:06, bradn wrote: > Keeping the 3 sorts of wasm frames orthogonal seems clearer to me. I don't mind either way, but if we keep them separate then they probably shouldn't inherit from WasmFrame. I'm simply mimicking what JavaScriptFrame does with OptimizedFrame and InterpretedFrame deriving from JavaScriptFrame, and is_java_script() returning true if it's one of those 3 types. https://codereview.chromium.org/1839843002/diff/1/src/frames.h#newcode800 src/frames.h:800: DCHECK(frame->is_java_script()); Compare to this, which is also true for OptimizedFrame and InterpreterFrame.
On 2016/03/29 00:24:06, JF wrote: > LMK which way you'd rather go. The current code is different from the > pre-existing code, and I already thought these frames were confusing :-) > I agree with bradn@; I think is_wasm() should only return true for the WASM frames, as the other wrapper frames are more like stubs. > https://codereview.chromium.org/1839843002/diff/1/src/frames.h > File src/frames.h (right): > > https://codereview.chromium.org/1839843002/diff/1/src/frames.h#newcode417 > src/frames.h:417: bool is_wasm() const { > On 2016/03/28 23:46:06, bradn wrote: > > Keeping the 3 sorts of wasm frames orthogonal seems clearer to me. > > I don't mind either way, but if we keep them separate then they probably > shouldn't inherit from WasmFrame. I'm simply mimicking what JavaScriptFrame does > with OptimizedFrame and InterpretedFrame deriving from JavaScriptFrame, and > is_java_script() returning true if it's one of those 3 types. > Agree. The WASM wrapper frames shouldn't be considered "real wasm" in the same way that stub frames are not considered "real JS". > https://codereview.chromium.org/1839843002/diff/1/src/frames.h#newcode800 > src/frames.h:800: DCHECK(frame->is_java_script()); > Compare to this, which is also true for OptimizedFrame and InterpreterFrame.
Did some pair-programming on the bus with Brad. This update makes the inheritance + hand-implemented dynamic_cast consistent.
Description was changed from ========== wasm: is_wasm fix wasm_to_js and js_to_wasm both derive from wasm, it seems confusing to not have is_wasm be true for both of them. I bumped into this when adding WasmFrame::cast. R=bradnelson@chromium.org, titzer@chromium.org ========== to ========== wasm: implemente WasmFrame::cast, fix inheritance wasm_to_js and js_to_wasm both derive from wasm, which was confusing because is_wasm wasn't true for them and that made WasmFrame::cast awkward. Make them derive from StubFrame instead. R=bradnelson@chromium.org, titzer@chromium.org ==========
The CQ bit was checked by jfb@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1839843002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1839843002/20001
https://codereview.chromium.org/1839843002/diff/20001/src/frames.cc File src/frames.cc (right): https://codereview.chromium.org/1839843002/diff/20001/src/frames.cc#newcode769 src/frames.cc:769: if (!is_wasm() || is_wasm_to_js()) { don't do that...
https://codereview.chromium.org/1839843002/diff/20001/src/frames.cc File src/frames.cc (right): https://codereview.chromium.org/1839843002/diff/20001/src/frames.cc#newcode769 src/frames.cc:769: if (!is_wasm() || is_wasm_to_js()) { On 2016/03/29 17:06:57, bradnelson wrote: > don't do that... Thought I'd backed that off.
The CQ bit was checked by jfb@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1839843002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1839843002/40001
lgtm
On 2016/03/29 17:11:48, bradnelson wrote: > lgtm lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by jfb@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1839843002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1839843002/40001
Message was sent while issue was closed.
Description was changed from ========== wasm: implemente WasmFrame::cast, fix inheritance wasm_to_js and js_to_wasm both derive from wasm, which was confusing because is_wasm wasn't true for them and that made WasmFrame::cast awkward. Make them derive from StubFrame instead. R=bradnelson@chromium.org, titzer@chromium.org ========== to ========== wasm: implemente WasmFrame::cast, fix inheritance wasm_to_js and js_to_wasm both derive from wasm, which was confusing because is_wasm wasn't true for them and that made WasmFrame::cast awkward. Make them derive from StubFrame instead. R=bradnelson@chromium.org, titzer@chromium.org ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== wasm: implemente WasmFrame::cast, fix inheritance wasm_to_js and js_to_wasm both derive from wasm, which was confusing because is_wasm wasn't true for them and that made WasmFrame::cast awkward. Make them derive from StubFrame instead. R=bradnelson@chromium.org, titzer@chromium.org ========== to ========== wasm: implemente WasmFrame::cast, fix inheritance wasm_to_js and js_to_wasm both derive from wasm, which was confusing because is_wasm wasn't true for them and that made WasmFrame::cast awkward. Make them derive from StubFrame instead. R=bradnelson@chromium.org, titzer@chromium.org Committed: https://crrev.com/18b44702126d9b27f954f7c9b7168716369b0478 Cr-Commit-Position: refs/heads/master@{#35115} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/18b44702126d9b27f954f7c9b7168716369b0478 Cr-Commit-Position: refs/heads/master@{#35115} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
