[webkit-reviews] review denied: [Bug 132755] WebKit2 on iOS needs to capture the main thread's floating point environment : [Attachment 231324] patch 3: removed an unneeded #import <fenv.h> in WebCoreThread.h

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has denied Mark Lam <mark.lam at apple.com>'s
request for review:
Bug 132755: WebKit2 on iOS needs to capture the main thread's floating point
environment
https://bugs.webkit.org/show_bug.cgi?id=132755

Attachment 231324: patch 3: removed an unneeded #import <fenv.h> in
WebCoreThread.h
https://bugs.webkit.org/attachment.cgi?id=231324&action=review

------- Additional Comments from Geoffrey Garen <ggaren at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=231321&action=review


> Source/WebCore/platform/ios/wak/FloatingPointEnv.cpp:47
> +void FloatingPointEnv::enableNeededFloatingPointModes()

This is a needlessly abstract name. Also, it's a false name: This function
enables only one mode. Notice that, due to this bad name, you had to add a
comment in your change log explaining that you called this function "to support
denormalized numbers". A better way to indicate that is to put it in the
function's name.

> Source/WebCore/platform/ios/wak/FloatingPointEnv.cpp:54
> +    env.__fpscr &= ~0x01000000U; // Enable denormalized numbers.

This comment duplicates the comment above it.

You should add a comment explaining that this state is the default on x86_64
and ARM64.

> Source/WebCore/platform/ios/wak/FloatingPointEnv.cpp:59
> +    RELEASE_ASSERT((env.__mxcsr & 0x1f80) == 0x1f80); // Ensure FP
exceptions are disabled.

Let's not ASSERT this. This ASSERT would not have caught our bug because it
only run on the main thread, and it run before a secondary thread might read
the relevant data.

Also, though it would be strange, we can't guarantee that a WebKit1 client
process did not enable FP exceptions. Perhaps they did.

I would suggest reworking this ASSERT, but it's actually already better covered
by the m_isInitialized ASSERT, since that tests directly for passing
uninitialized data to fsetenv. So, let's just remove this ASSERT.

> Source/WebCore/platform/ios/wak/FloatingPointEnv.h:35
> +class FloatingPointEnv {

I think we can afford to spell out "Environment" here. Also, you used
"Environment" in the static functions below, and consistency is a goal.

> Source/WebCore/platform/ios/wak/FloatingPointEnv.h:41
> +    static void enableNeededFloatingPointModes();
> +    static void saveMainThreadEnvironment();
> +    static void propagateMainThreadEnvironment();

These functions should be member functions, and the accessor to the shared
floating point environment object should be public.

It doesn't make sense to mix all static functions with per-object data.