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

[bugzilla-daemon at webkit.org]

Mark Lam <mark.lam at apple.com> has denied 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 349756: Patch

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




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

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

> Source/JavaScriptCore/assembler/MacroAssemblerPrinter.cpp:140
> +	   // assuming memory is not malformed, it originately pointed to a
value

/originately/originally/

> Source/JavaScriptCore/interpreter/Interpreter.cpp:814
> +	   IGNORE_CAST_ALIGN_WARNINGS_BEGIN
>	   EXCEPTION_ASSERT(throwScope.exception() ==
reinterpret_cast<Exception*>(error));
> +	   IGNORE_CAST_ALIGN_WARNINGS_END

Instead of doing this, can you just change prepareForExecution() to return a
std::optional<Exception*>?  That should make the code cleaner and better
document what will actually be returned.  That also removes the need for all
these reinterpret_casts.  I think the JSObject* return value is a holdover from
ancient times.

> Source/bmalloc/bmalloc/IsoPageInlines.h:197
> +	   // the cast below is safe on these platforms as long as
> +	   // both Config::objectSize and the alignment of this are multiples
of
> +	   // the required alignment of FreeCell
> +	   static_assert(!(Config::objectSize % alignof(FreeCell)),
"Config::objectSize should respect alignment of FreeCell");
> +	   static_assert(!(alignof(IsoPage<Config>) % alignof(FreeCell)),
"Alignment of IsoPage<Config> should match that of FreeCell");

Shouldn't these be always true?  I think we can get rid of the #if CPU(ARM) ||
CPU(MIPS) here and below.  You can also remove the comment.  The assertions are
sufficient to document the invariant / requirement.