[webkit-reviews] review denied: [Bug 49112] Fix 32-bit vs. 64-bit Accelerate.framework issues in VectorMath : [Attachment 73139] Patch

[bugzilla-daemon at webkit.org]

Maciej Stachowiak <mjs at apple.com> has denied Chris Rogers
<crogers at google.com>'s request for review:
Bug 49112: Fix 32-bit vs. 64-bit Accelerate.framework issues in VectorMath
https://bugs.webkit.org/show_bug.cgi?id=49112

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

------- Additional Comments from Maciej Stachowiak <mjs at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=73139&action=review

What is the effect of this patch? Does the current code fail to build on
32-bit, or does it build but behave differently when changed this way? Is there
a reason we can't use the same code path for both 32-bit and 64-bit platforms?

Also of note: ARM is a 32-bit platform not covered by your macros. Is it ok for
it to go down the same code path as 64-bit? I think either your macro test is
wrong (if ARM should be included with 32-bit)) or the ChangeLog is wrong (if
the issue is specific to just i386 and ppc, rather than all 32-bit platforms).

> WebCore/platform/audio/VectorMath.cpp:41
> +// In 32-bit mode __ppc__ or __i386__ is defined and
<vecLib/vDSP_translate.h> is included which defines macros, so we must handle
this case differently.

This comment does not seem useful. It says the 32-bit case is handled
differently, but does not say why.