[webkit-reviews] review granted: [Bug 188598] [JSC] Remove gcc warnings on mips and armv7 : [Attachment 351282] Patch

[bugzilla-daemon at webkit.org]

Mark Lam <mark.lam at apple.com> has granted Guillaume Emont
<guijemont at igalia.com>'s request for review:
Bug 188598: [JSC] Remove gcc warnings on mips and armv7
https://bugs.webkit.org/show_bug.cgi?id=188598

Attachment 351282: Patch

https://bugs.webkit.org/attachment.cgi?id=351282&action=review




--- Comment #27 from Mark Lam <mark.lam at apple.com> ---
Comment on attachment 351282
  --> https://bugs.webkit.org/attachment.cgi?id=351282
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=351282&action=review

r=me.  The patch looks correct though I think it would be better if we use
static_casts in instead of bitwise_casts.

> Source/JavaScriptCore/runtime/ScriptExecutable.cpp:349
> -	   return throwException(&state, throwScope, createError(&state,
"Forced Failure"_s));
> +	   return std::optional<Exception
*>(bitwise_cast<Exception*>(throwException(&state, throwScope,
createError(&state, "Forced Failure"_s))));

I actually disagree with using bitwise_cast here because we're casting from a
pointer to a pointer.  Hence, they are guaranteed to be of the same bit length.
 In fact, I think a static cast would be more meaningful because it enforces
that there's a "is a" relationship between the source (JSObject*) and the
result (Exception*).  The times when we should use bitwise_cast are when
casting between the bit value of non-pointer types (e.g. integers) and
pointers.

Are you sure that this is where Yusuke is asking to apply the bitwise_cast?

> Source/JavaScriptCore/runtime/ScriptExecutable.cpp:357
> +	   return std::optional<Exception
*>(bitwise_cast<Exception*>(exception));

Ditto.