Get rid of it.
Created attachment 245969 [details] the patch
Comment on attachment 245969 [details] the patch r=me
(In reply to comment #2) > Comment on attachment 245969 [details] > the patch > > r=me Invalid patch, will upload a new one.
Created attachment 245970 [details] the patch
Created attachment 245971 [details] the patch
Comment on attachment 245971 [details] the patch r=me
(In reply to comment #6) > Comment on attachment 245971 [details] > the patch > > r=me Lol, my patch is still wrong. ;-)
Created attachment 246035 [details] the patch For real this time!
Comment on attachment 246035 [details] the patch View in context: https://bugs.webkit.org/attachment.cgi?id=246035&action=review > Source/JavaScriptCore/ChangeLog:47 > +2015-02-03 Filip Pizlo <fpizlo@apple.com> > + > + Reviewed by NOBODY (OOPS!). > + > + Also removes the localArgumentsRegister local variable in BytecodeGenerator's > + constructor. It's not used. > + > + * bytecompiler/BytecodeGenerator.cpp: > + (JSC::BytecodeGenerator::BytecodeGenerator): > + (JSC::BytecodeGenerator::preserveLastVar): Deleted. > + * bytecompiler/BytecodeGenerator.h: > + (JSC::BytecodeGenerator::addVar): > + Leftover edit that should have been removed? > Source/JavaScriptCore/bytecompiler/RegisterID.h:75 > - ASSERT(!m_refCount); > #ifndef NDEBUG > m_didSetIndex = true; > #endif I think the main reason for the ASSERT(!m_refCount) is to ensure that the index is only set once before use. Though the previous implementation allows us to “change our minds” and set the index to something else (which I don’t think is likely to happen) as long as we haven’t started using the RegisterID yet, I think the real bug it’s trying to watch out for is that we don’t use the RegisterID’s index in one place and then change it before using it in another (which is also unlikely to happen but if it does, it can cause havoc). How about you replace this assert with ASSERT(!m_didSetIndex) in the #ifndef NDEBUG section instead? Would that work?
(In reply to comment #9) > Comment on attachment 246035 [details] > the patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=246035&action=review > > > Source/JavaScriptCore/ChangeLog:47 > > +2015-02-03 Filip Pizlo <fpizlo@apple.com> > > + > > + Reviewed by NOBODY (OOPS!). > > + > > + Also removes the localArgumentsRegister local variable in BytecodeGenerator's > > + constructor. It's not used. > > + > > + * bytecompiler/BytecodeGenerator.cpp: > > + (JSC::BytecodeGenerator::BytecodeGenerator): > > + (JSC::BytecodeGenerator::preserveLastVar): Deleted. > > + * bytecompiler/BytecodeGenerator.h: > > + (JSC::BytecodeGenerator::addVar): > > + > > Leftover edit that should have been removed? Yup, I removed it locally. > > > Source/JavaScriptCore/bytecompiler/RegisterID.h:75 > > - ASSERT(!m_refCount); > > #ifndef NDEBUG > > m_didSetIndex = true; > > #endif > > I think the main reason for the ASSERT(!m_refCount) is to ensure that the > index is only set once before use. Though the previous implementation > allows us to “change our minds” and set the index to something else (which I > don’t think is likely to happen) as long as we haven’t started using the > RegisterID yet, I think the real bug it’s trying to watch out for is that we > don’t use the RegisterID’s index in one place and then change it before > using it in another (which is also unlikely to happen but if it does, it can > cause havoc). > > How about you replace this assert with ASSERT(!m_didSetIndex) in the #ifndef > NDEBUG section instead? Would that work? But this does get called when m_didSetIndex is true.
Comment on attachment 246035 [details] the patch View in context: https://bugs.webkit.org/attachment.cgi?id=246035&action=review r=me with ChangeLog fix. >>> Source/JavaScriptCore/bytecompiler/RegisterID.h:75 >>> #endif >> >> I think the main reason for the ASSERT(!m_refCount) is to ensure that the index is only set once before use. Though the previous implementation allows us to “change our minds” and set the index to something else (which I don’t think is likely to happen) as long as we haven’t started using the RegisterID yet, I think the real bug it’s trying to watch out for is that we don’t use the RegisterID’s index in one place and then change it before using it in another (which is also unlikely to happen but if it does, it can cause havoc). >> >> How about you replace this assert with ASSERT(!m_didSetIndex) in the #ifndef NDEBUG section instead? Would that work? > > But this does get called when m_didSetIndex is true. OK. For Filip explained offline that for a scenario like: function(a) { var a; }, the var a overrides argument a. As such, the index for a’s RegisterID will be changed to match that of the argument’s. Hence, m_didSetIndex is already true there.
(In reply to comment #11) > OK. For Filip explained offline that ... I meant: For the record, Filip explained ... Ahh typos.
Landed in http://trac.webkit.org/changeset/179746