[webkit-reviews] review denied: [Bug 132330] LLINT loadisFromInstruction doesn't need special case for big endians : [Attachment 230365] Proposed patch

[bugzilla-daemon at webkit.org]

Mark Lam <mark.lam at apple.com> has denied Tomas Popela <tpopela at redhat.com>'s
request for review:
Bug 132330: LLINT loadisFromInstruction doesn't need special case for big
endians
https://bugs.webkit.org/show_bug.cgi?id=132330

Attachment 230365: Proposed patch
https://bugs.webkit.org/attachment.cgi?id=230365&action=review

------- Additional Comments from Mark Lam <mark.lam at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=230365&action=review


Thanks for fixing this.  If you update the ChangeLog comment, we should be good
to go.

> Source/JavaScriptCore/ChangeLog:9
> +	   The change introduced in r167076 was wrong. We cannot apply the
offset
> +	   adjustment on every loadisFromInstruction usage as the instruction

Please change “cannot” to “should not” and remove “every”.  As it read
previously, it implied that there are some cases where the offset should be
applied, which is not true.  It is always wrong to applied that offset.

> Source/JavaScriptCore/ChangeLog:11
> +	   operand variable). The offset of the union members will be the

Please change “union” to “other union”.

> Source/JavaScriptCore/ChangeLog:13
> +	   same on little and big endian architectures thus we don't need

Missing period between “architectures. Thus”.