Summary: | We should be able to jsDynamicCast from JSType when possible | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Keith Miller <keith_miller> | ||||||||||||||
Component: | New Bugs | Assignee: | Keith Miller <keith_miller> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | commit-queue, ews-watchlist, fpizlo, mark.lam, msaboff, saam, webkit-bug-importer | ||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||
OS: | Unspecified | ||||||||||||||||
Attachments: |
|
Description
Keith Miller
2018-02-14 12:58:13 PST
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. |