Canonicalize aquiring the JSCell lock.
Created attachment 332741 [details] Patch
Created attachment 332762 [details] Patch
Created attachment 332763 [details] Patch
Comment on attachment 332763 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=332763&action=review > Source/JavaScriptCore/runtime/JSCell.h:124 > + // It's also recommended that cells are locked with the holdJSCellLock() method to help keep Why not make those private and friend lockholder then? Or you could do what I suggest below and make these private and people can call lock/unlock on the wrapper type > Source/JavaScriptCore/runtime/JSCell.h:126 > + Locker<JSCell> holdJSCellLock() { return holdLock(*this); } Not in love with the name here. What if you had some method named “cellLock” and it returned to you some wrapper that exposes lock/unlock methods. You could do: auto locker = holdLock(cell->cellLock()) But I’m not sure if that’s much better
Comment on attachment 332763 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=332763&action=review >> Source/JavaScriptCore/runtime/JSCell.h:124 >> + // It's also recommended that cells are locked with the holdJSCellLock() method to help keep > > Why not make those private and friend lockholder then? Or you could do what I suggest below and make these private and people can call lock/unlock on the wrapper type You can't really do that. If you friend Locker then anyone can make a Locker that can lock a JSCell so we're in the same boat. You also can't make a subclass of Locker that you friend because the base class needs access to lock/unlock. >> Source/JavaScriptCore/runtime/JSCell.h:126 >> + Locker<JSCell> holdJSCellLock() { return holdLock(*this); } > > Not in love with the name here. What if you had some method named “cellLock” and it returned to you some wrapper that exposes lock/unlock methods. You could do: > auto locker = holdLock(cell->cellLock()) > But I’m not sure if that’s much better Yeah, I guess that's a bit better in that you can't just holdLock(cell).
Created attachment 332773 [details] Patch
Created attachment 332775 [details] Patch
Comment on attachment 332775 [details] Patch r=me after build failures.
(In reply to Michael Saboff from comment #8) > Comment on attachment 332775 [details] > Patch > > r=me after build failures. Looks like ToT is broken...
Comment on attachment 332775 [details] Patch Clearing flags on attachment: 332775 Committed r227906: <https://trac.webkit.org/changeset/227906>
All reviewed patches have been landed. Closing bug.
<rdar://problem/37077485>
Comment on attachment 332775 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=332775&action=review > Source/JavaScriptCore/runtime/JSCell.h:288 > +class JSCellLock : public JSCell { nice. This patch LGTM too. I think this API works out nicely.