Bug 233415 – [JSC] Move m_incomingPolymorphicCalls out of CodeBlock::JITData

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Yusuke Suzuki]

[JSC] Move m_incomingPolymorphicCalls out of CodeBlock::JITData

[Yusuke Suzuki]

Created attachment 444946 [details]
Patch

[Yusuke Suzuki]

Created attachment 444947 [details]
Patch

[Yusuke Suzuki]

Created attachment 444949 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/85801379>

[Mark Lam]

Comment on attachment 444949 [details]
Patch

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

Nice.  r=me

> Tools/TestWebKitAPI/Tests/WTF/SentinelLinkedList.cpp:96
> +

Can you add the following here?
    EXPECT_TRUE(!first->isOnList());
    EXPECT_TRUE(!second->isOnList());
    EXPECT_TRUE(!third->isOnList());
    EXPECT_TRUE(!fourth->isOnList());

> Tools/TestWebKitAPI/Tests/WTF/SentinelLinkedList.cpp:100
> +TEST(WTF_SentinelLinkedList, RemoveAll)

I think `Remove` is a better name for this test (instead of `RemoveAll`) since it's testing the `remove` method.

> Tools/TestWebKitAPI/Tests/WTF/SentinelLinkedList.cpp:115
> +}

Can you add an `EXPECT_TRUE(i == 10);` here?

[Darin Adler]

Comment on attachment 444949 [details]
Patch

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

>> Tools/TestWebKitAPI/Tests/WTF/SentinelLinkedList.cpp:115
>> +}
> 
> Can you add an `EXPECT_TRUE(i == 10);` here?

Or more idiomatically:

    EXPECT_EQ(10, i);

> Tools/TestWebKitAPI/Tests/WTF/SentinelLinkedList.cpp:132
> +    EXPECT_EQ(list.begin()->value(), 0);

These are all backward. The macro writes out error messages assuming that the expected value is first and the expression is second. You’ll notice this in failing tests, so it’s slightly more elegant to do it in the order the macro exprects.

[Yusuke Suzuki]

Comment on attachment 444949 [details]
Patch

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

Thank you!

>> Tools/TestWebKitAPI/Tests/WTF/SentinelLinkedList.cpp:96
>> +
> 
> Can you add the following here?
>     EXPECT_TRUE(!first->isOnList());
>     EXPECT_TRUE(!second->isOnList());
>     EXPECT_TRUE(!third->isOnList());
>     EXPECT_TRUE(!fourth->isOnList());

Nice. Added.

>> Tools/TestWebKitAPI/Tests/WTF/SentinelLinkedList.cpp:100
>> +TEST(WTF_SentinelLinkedList, RemoveAll)
> 
> I think `Remove` is a better name for this test (instead of `RemoveAll`) since it's testing the `remove` method.

Make sense. Changed.

>>> Tools/TestWebKitAPI/Tests/WTF/SentinelLinkedList.cpp:115
>>> +}
>> 
>> Can you add an `EXPECT_TRUE(i == 10);` here?
> 
> Or more idiomatically:
> 
>     EXPECT_EQ(10, i);

Sounds good. Added.

>> Tools/TestWebKitAPI/Tests/WTF/SentinelLinkedList.cpp:132
>> +    EXPECT_EQ(list.begin()->value(), 0);
> 
> These are all backward. The macro writes out error messages assuming that the expected value is first and the expression is second. You’ll notice this in failing tests, so it’s slightly more elegant to do it in the order the macro exprects.

Good catch! Changed :)

[Yusuke Suzuki]

Committed r286248 (244611@main): <https://commits.webkit.org/244611@main>