We should be able to jsDynamicCast from JSType when possible
Created attachment 333832 [details] WIP
Created attachment 333851 [details] Patch
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.
Created attachment 333853 [details] Patch
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.
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.
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.
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.
Created attachment 333858 [details] Patch
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.
Created attachment 333861 [details] Patch for landing
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?
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
Created attachment 333862 [details] Patch for landing
Comment on attachment 333862 [details] Patch for landing Clearing flags on attachment: 333862 Committed r228500: <https://trac.webkit.org/changeset/228500>
All reviewed patches have been landed. Closing bug.
<rdar://problem/37556223>