Bug 182804 – We should be able to jsDynamicCast from JSType when possible

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

[Keith Miller]

We should be able to jsDynamicCast from JSType when possible

[Keith Miller]

Created attachment 333832 [details]
WIP

[Keith Miller]

Created attachment 333851 [details]
Patch

[EWS Watchlist]

Attachment 333851 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/JSCast.h:36:  Please replace ASSERT_WITH_SECURITY_IMPLICATION() with RELEASE_ASSERT_WITH_SECURITY_IMPLICATION().  [security/assertion] [5]
ERROR: Source/JavaScriptCore/runtime/JSCast.h:44:  Please replace ASSERT_WITH_SECURITY_IMPLICATION() with RELEASE_ASSERT_WITH_SECURITY_IMPLICATION().  [security/assertion] [5]
ERROR: Source/JavaScriptCore/runtime/JSCast.h:109:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 3 in 53 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Keith Miller]

Created attachment 333853 [details]
Patch

[Mark Lam]

Comment on attachment 333851 [details]
Patch

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

r=me me with build fixes.  You probably have to fix some #includes in WebCore and CodeGeneratorJS.pm.

> Source/JavaScriptCore/runtime/JSCast.h:35
> +    static_assert(std::is_base_of<JSCell, typename std::remove_pointer<To>::type>::value && std::is_base_of<JSCell, typename std::remove_pointer<From>::type>::value, "JS casting expects that the types you are casting to/from are subclasses of JSCells");

I suggest /JSCells/JSCell/.  No need for plural because there is only one JSCell class that these are subclasses of.

> Source/JavaScriptCore/runtime/JSCast.h:58
> +#define FORWARD_DECLARE_OVERLOAD_CLASS(className, jsType, op) class className;

I recommend you replace /jsType/unused1/ and /op/unused2/.  "op" is what caught my eye because the value passed in there is no some sort of op.

> Source/JavaScriptCore/runtime/JSCast.h:68
> +    static_assert(std::is_base_of<JSCell, typename std::remove_pointer<To>::type>::value && std::is_base_of<JSCell, typename std::remove_pointer<From>::type>::value, "JS casting expects that the types you are casting to/from are subclasses of JSCells");

Ditto /JSCells/JSCell/.

> Source/JavaScriptCore/runtime/JSCast.h:109
> +    return Dispatcher::template cast<To>(vm, from);;

Remove extra semicolon.

> Source/JavaScriptCore/runtime/JSFunction.cpp:110
>      ASSERT(inherits(vm, info()));
> +    ASSERT(jsDynamicCast<JSFunction*>(vm, this));

These 2 asserts are redundant.  I suggest removing the inherits one since the jsDynamicCast is clearer to read.

[EWS Watchlist]

Attachment 333853 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/JSCast.h:36:  Please replace ASSERT_WITH_SECURITY_IMPLICATION() with RELEASE_ASSERT_WITH_SECURITY_IMPLICATION().  [security/assertion] [5]
ERROR: Source/JavaScriptCore/runtime/JSCast.h:44:  Please replace ASSERT_WITH_SECURITY_IMPLICATION() with RELEASE_ASSERT_WITH_SECURITY_IMPLICATION().  [security/assertion] [5]
ERROR: Source/JavaScriptCore/runtime/JSCast.h:109:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 3 in 53 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Mark Lam]

Comment on attachment 333853 [details]
Patch

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

> Source/JavaScriptCore/runtime/InternalFunction.cpp:51
>      ASSERT(inherits(vm, info()));

Can remove this.  It's now redundant.

> Source/JavaScriptCore/runtime/JSFunction.cpp:109
>      ASSERT(inherits(vm, info()));

Remove.  redundant.

> Source/JavaScriptCore/runtime/JSObject.h:880
>          ASSERT(inherits(vm, info()));

Remove.  Redundant.

[Filip Pizlo]

Comment on attachment 333853 [details]
Patch

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

r=me assuming you put out the bot fire.

> Source/JavaScriptCore/ChangeLog:10
> +        This patch beefs up jsDynamicCast in some of the cases where we
> +        can use the JSType to quickly determine if a cell is a subclass of
> +        the desired type.

you should say more about the mechanism, like the type range check.

[Keith Miller]

Created attachment 333858 [details]
Patch

[EWS Watchlist]

Attachment 333858 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/JSCast.h:36:  Please replace ASSERT_WITH_SECURITY_IMPLICATION() with RELEASE_ASSERT_WITH_SECURITY_IMPLICATION().  [security/assertion] [5]
ERROR: Source/JavaScriptCore/runtime/JSCast.h:44:  Please replace ASSERT_WITH_SECURITY_IMPLICATION() with RELEASE_ASSERT_WITH_SECURITY_IMPLICATION().  [security/assertion] [5]
Total errors found: 2 in 54 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Keith Miller]

Created attachment 333861 [details]
Patch for landing

[Mark Lam]

Comment on attachment 333861 [details]
Patch for landing

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

> Source/JavaScriptCore/runtime/JSCast.h:35
> +    static_assert(std::is_base_of<JSCell, typename std::remove_pointer<To>::type>::value && std::is_base_of<JSCell, typename std::remove_pointer<From>::type>::value, "JS casting expects that the types you are casting to/from are subclasses of JSCells");

JSCells?

[Saam Barati]

Comment on attachment 333861 [details]
Patch for landing

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

LGTM too

> Source/JavaScriptCore/runtime/JSCast.h:53
> +    macro(JSArray, JSType::ArrayType, JSType::DerivedArrayType) \

Might be worth a comment in JSType.h that these need to be adjacent. Or perhaps a static_assert here

[Keith Miller]

Created attachment 333862 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 333862 [details]
Patch for landing

Clearing flags on attachment: 333862

Committed r228500: <https://trac.webkit.org/changeset/228500>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/37556223>