[webkit-reviews] review granted: [Bug 41177] References for movable objects : [Attachment 62985] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Nathan Lawrence
<nlawrence at apple.com>'s request for review:
Bug 41177: References for movable objects
https://bugs.webkit.org/show_bug.cgi?id=41177

Attachment 62985: Patch
https://bugs.webkit.org/attachment.cgi?id=62985&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> -		   markStack.append(reinterpret_cast<JSCell*>(xAsBits));
> +		   // While markStack.append normally takes a reference to
update,
> +		   // we don't actually want the heap to be updated since we
don't
> +		   // know for sure that it's actually a pointer.  In the
future
> +		   // this will be replaced by some appendRoot function for
this
> +		   // specific case.
> +		   JSCell* cell = reinterpret_cast<JSCell*>(xAsBits);
> +		   markStack.append(cell);

Thanks. I find this comment really helpful. We normally use one space after
periods, not two.

> +	   // In this case, we need to be able to change the pointer, and
although we know
> +	   // it to be safe, C++ doesn't, requiring us to use templated
functions that
> +	   // pass a casted version to an internal function.

It's too bad you didn't give the reason we know it to be safe. Otherwise
excellent!

> +    ALWAYS_INLINE void MarkStack::append(Register& reg)

Hi, Reg! I guess it's short for Reginald ;-)

>  #include "JSValue.h"
> +#include "Register.h"
>  #include <wtf/Noncopyable.h>

Why are you adding an include of Register.h to MarkStack.h? I don't see
anything in this patch that requires that.

r=me

Please see if you can land this without adding that Register.h include.