...
Created attachment 259665 [details] Patch
Created attachment 259677 [details] Patch
Comment on attachment 259677 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=259677&action=review Add blank lines between function declarations or definitions. Let's use = initialization for "for" loops. r=me with some comment clean up and stylistic changes. > Source/JavaScriptCore/jit/CallFrameShuffleData.cpp:43 > + for (size_t i { 0 }; i < registerSaveLocations->size(); ++i) { Let's use = initialization for "for" loops. > Source/JavaScriptCore/jit/CallFrameShuffleData.cpp:55 > + for (Reg reg { Reg::first() }; reg <= Reg::last(); reg = reg.next()) { Let's use = initialization for "for" loops. > Source/JavaScriptCore/jit/CallFrameShuffler.h:41 > +// A CachedRecovery is a simple wrapper around a ValueRecovery that That what? Complete the comment. > Source/JavaScriptCore/jit/CallFrameShuffler.h:106 > + Delete one blank line. > Source/JavaScriptCore/jit/CallFrameShuffler.h:451 > + int m_frameDelta { m_alignedNewFrameSize - m_alignedOldFrameSize }; Initialize this in the constructor. > Source/JavaScriptCore/jit/CallFrameShuffler.h:685 > + // pointer, and m_oldFrameOffset is 0. We could We could what? Finish the comment.
Comment on attachment 259677 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=259677&action=review Addressed the comments below + added blank lines between method definitions. >> Source/JavaScriptCore/jit/CallFrameShuffleData.cpp:43 >> + for (size_t i { 0 }; i < registerSaveLocations->size(); ++i) { > > Let's use = initialization for "for" loops. Changed all of those. >> Source/JavaScriptCore/jit/CallFrameShuffler.h:41 >> +// A CachedRecovery is a simple wrapper around a ValueRecovery that > > That what? Complete the comment. Oh, oops. I should actually reword it since it's a bit more than a "simple wrapper". Changed to "A CachedRecovery is a wrapper around a ValueRecovery that records where said value should go on the stack and/or in registers. Whenever we perform an operation changing the ValueRecovery, we update the CachedRecovery's member in place." >> Source/JavaScriptCore/jit/CallFrameShuffler.h:106 >> + > > Delete one blank line. Done. >> Source/JavaScriptCore/jit/CallFrameShuffler.h:451 >> + int m_frameDelta { m_alignedNewFrameSize - m_alignedOldFrameSize }; > > Initialize this in the constructor. I did this here because I believed it to make it clearer what happens when reading the comment. Moved to the constructor. >> Source/JavaScriptCore/jit/CallFrameShuffler.h:685 >> + // pointer, and m_oldFrameOffset is 0. We could > > We could what? Finish the comment. I'll just remove the "We could" part. I wanted to say that we could use the stack pointer as well, but that'd just be noise.
Created attachment 259806 [details] Patch
Comment on attachment 259806 [details] Patch r=me
Committed r188937 <http://trac.webkit.org/changeset/188937>