...
Created attachment 400012 [details] patch
Comment on attachment 400012 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=400012&action=review > JSTests/stress/in-by-val-inside-for-in-loop.js:35 > + if (p === "a") > + delete o.a; Since you're using the same o object every time, this delete will only take place on the first time thru here. Is that intentional? > JSTests/stress/in-by-val-inside-for-in-loop.js:110 > + delete o[p]; Ditto. We only delete the properties in o on the first run thru here. Is this intentional? > JSTests/stress/in-by-val-inside-for-in-loop.js:144 > + delete o[p]; Ditto.
Comment on attachment 400012 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=400012&action=review >> JSTests/stress/in-by-val-inside-for-in-loop.js:35 >> + delete o.a; > > Since you're using the same o object every time, this delete will only take place on the first time thru here. Is that intentional? yes. Just to change structure
Comment on attachment 400012 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=400012&action=review > Source/JavaScriptCore/ChangeLog:26 > + uses the HasProperty internal method, instead of like op_has_structure_property, s/instead of like/unlike/ ? > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:5203 > + static_assert(std::is_same<NewOpType, OpGetByVal>::value || std::is_same<NewOpType, OpInByVal>::value, "get_by_val and in_by_val have arguments in the same order."); If this must be true it might be worth placing them in an op_group in BytecodeList.rb and adding a comment. Eventually we should add a way of referring to the group itself for cases like this. > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:5216 > + generator.m_lastOpcodeID = op_end; also pre-existing, but we should make a method for this. > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:5225 > + while (generator.m_writer.position() < end) Assert that position <= end? I'm not sure if we have a static method for that, but might worth asserting that `OldOpType::length() <= NewOpType::length()`. One thing I'm not sure if it's worth it since there's only two use cases is not allowing an arbitrary pair <OldOpType, NewOpType>.
Created attachment 400061 [details] patch fix debug assert. I was casting to the wrong op type in baseline JIT compiler
Comment on attachment 400012 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=400012&action=review >> Source/JavaScriptCore/ChangeLog:26 >> + uses the HasProperty internal method, instead of like op_has_structure_property, > > s/instead of like/unlike/ ? I will fix >> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:5203 >> + static_assert(std::is_same<NewOpType, OpGetByVal>::value || std::is_same<NewOpType, OpInByVal>::value, "get_by_val and in_by_val have arguments in the same order."); > > If this must be true it might be worth placing them in an op_group in BytecodeList.rb and adding a comment. Eventually we should add a way of referring to the group itself for cases like this. But they don't share the same metadata. I can just remove the reliance on this in the code below >> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:5225 >> + while (generator.m_writer.position() < end) > > Assert that position <= end? I'm not sure if we have a static method for that, but might worth asserting that `OldOpType::length() <= NewOpType::length()`. One thing I'm not sure if it's worth it since there's only two use cases is not allowing an arbitrary pair <OldOpType, NewOpType>. static-assert below guarantees this. I'll move that static_assert into this function when adapting it to not rely on NewOpType
Created attachment 400069 [details] patch
Comment on attachment 400069 [details] patch r=me
Committed r262083: <https://trac.webkit.org/changeset/262083> All reviewed patches have been landed. Closing bug and clearing flags on attachment 400069 [details].
<rdar://problem/63556424>
Comment on attachment 400069 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=400069&action=review > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2491 > + OpInStructureProperty::emit<OpcodeSize::Wide32>(this, dst, base, property, structureContext.enumerator()); Why does this have to be wide? Is it just because it makes the rewriting easier? Seems like we should at least have a FIXME.