[JSC] Remove defaultValue() from the method table
Created attachment 427005 [details] Patch
Comment on attachment 427005 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=427005&action=review > Source/JavaScriptCore/API/JSCallbackObject.h:202 > + static EncodedJSValue customToPrimitiveImpl(JSGlobalObject*, CallFrame*); Not sure what the word "impl" means in this function name. Aren’t most functions "implementations"? How is this one different? > Source/JavaScriptCore/API/JSCallbackObjectFunctions.h:235 > + const JSCallbackObject* thisObject = jsDynamicCast<const JSCallbackObject*>(vm, callFrame->thisValue()); This is the kind of line of code where auto really shines, cutting down on repetitiveness and making this almost 1/3 shorter and easier to look over. > Source/JavaScriptCore/API/JSCallbackObjectFunctions.h:238 > + PreferredPrimitiveType hint = toPreferredPrimitiveType(globalObject, callFrame->argument(0)); Same. > Source/WebCore/bridge/runtime_object.cpp:187 > + return setUpSymbolToPrimitiveSlot<convertRuntimeObjectToPrimitive>(thisObject, lexicalGlobalObject, slot); We can hope that this Objective-C bridging is almost obsolete. Would be so nice to delete it some day. The value materializing only at this point is not something I would necessarily be OK with if this was still in active use, but I suspect it will be good enough to keep existing clients working. > Source/WebCore/bridge/runtime_object.cpp:228 > + const RuntimeObject* thisObject = jsDynamicCast<const RuntimeObject*>(vm, callFrame->thisValue()); auto? > Source/WebCore/bridge/runtime_object.cpp:231 > + RefPtr<Instance> instance = thisObject->getInternalInstance(); auto and makeRefPtr? > Source/WebCore/bridge/runtime_object.cpp:234 > + PreferredPrimitiveType hint = toPreferredPrimitiveType(lexicalGlobalObject, callFrame->argument(0)); auto? > Source/WebCore/bridge/runtime_object.h:94 > +template<RawNativeFunction toPrimitiveImpl> Not a fan of the "impl" here. > Source/WebCore/bridge/objc/objc_runtime.h:127 > + ObjcInstance* getInternalObjCInstance() const { return _instance.get(); } WebKit coding style recommends against the use of the word "get" in function names like this one. > Source/WebCore/bridge/objc/objc_runtime.mm:314 > + const ObjcFallbackObjectImp* thisObject = jsDynamicCast<const ObjcFallbackObjectImp*>(vm, callFrame->thisValue()); auto?
Created attachment 427013 [details] Patch Fix X86 build and npruntime tests, drop "impl" and lazy materialization, and use `auto` for WebCore.
(In reply to Darin Adler from comment #2) > Comment on attachment 427005 [details] > Patch Thank you for detailed comments, Darin! > > Source/JavaScriptCore/API/JSCallbackObject.h:202 > > + static EncodedJSValue customToPrimitiveImpl(JSGlobalObject*, CallFrame*); > > Not sure what the word "impl" means in this function name. Aren’t most > functions "implementations"? How is this one different? Removed the "impl". It was there because customToPrimitive() has the similar signature to callImpl() / constructImpl(), but unlike them, it's private and self-contained. > > Source/JavaScriptCore/API/JSCallbackObjectFunctions.h:235 > > + const JSCallbackObject* thisObject = jsDynamicCast<const JSCallbackObject*>(vm, callFrame->thisValue()); > > This is the kind of line of code where auto really shines, cutting down on > repetitiveness and making this almost 1/3 shorter and easier to look over. > > > Source/JavaScriptCore/API/JSCallbackObjectFunctions.h:238 > > + PreferredPrimitiveType hint = toPreferredPrimitiveType(globalObject, callFrame->argument(0)); > > Same. Removed the `const`, slightly cleaning up the code, but I am a little hesitant to use `auto` here. Unlike WebCore, JSC code base is a bit more type-explicit: in this file, `auto` is used only for throw scopes. > > Source/WebCore/bridge/runtime_object.cpp:187 > > + return setUpSymbolToPrimitiveSlot<convertRuntimeObjectToPrimitive>(thisObject, lexicalGlobalObject, slot); > > We can hope that this Objective-C bridging is almost obsolete. Would be so > nice to delete it some day. > > The value materializing only at this point is not something I would > necessarily be OK with if this was still in active use, but I suspect it > will be good enough to keep existing clients working. The purpose of lazy materialization was to avoid creating & putting JSFunction during construction. Since the bridging is almost obsolete, there is no point to (micro) optimize. Removed. > > Source/WebCore/bridge/runtime_object.cpp:228 > > + const RuntimeObject* thisObject = jsDynamicCast<const RuntimeObject*>(vm, callFrame->thisValue()); > > auto? > > > Source/WebCore/bridge/runtime_object.cpp:231 > > + RefPtr<Instance> instance = thisObject->getInternalInstance(); > > auto and makeRefPtr? > > > Source/WebCore/bridge/runtime_object.cpp:234 > > + PreferredPrimitiveType hint = toPreferredPrimitiveType(lexicalGlobalObject, callFrame->argument(0)); > > auto? Changed. > > Source/WebCore/bridge/runtime_object.h:94 > > +template<RawNativeFunction toPrimitiveImpl> > > Not a fan of the "impl" here. Removed the whole helper. > > Source/WebCore/bridge/objc/objc_runtime.h:127 > > + ObjcInstance* getInternalObjCInstance() const { return _instance.get(); } > > WebKit coding style recommends against the use of the word "get" in function names like this one. It does, but wouldn't it be nice to make ObjcFallbackObjectImp::getInternalObjCInstance() consistent with its friends? ObjCRuntimeObject::getInternalObjCInstance() CRuntimeObject::getInternalCInstance() RuntimeObject::getInternalInstance() > > Source/WebCore/bridge/objc/objc_runtime.mm:314 > > + const ObjcFallbackObjectImp* thisObject = jsDynamicCast<const ObjcFallbackObjectImp*>(vm, callFrame->thisValue()); > > auto? Changed.
Created attachment 427040 [details] Patch Simplify RuntimeObject::getOwnPropertySlot() and tweak ChangeLog.
Comment on attachment 427040 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=427040&action=review > Source/JavaScriptCore/API/JSCallbackObjectFunctions.h:238 > + JSCallbackObject* thisObject = jsDynamicCast<JSCallbackObject*>(vm, callFrame->thisValue()); > + if (!thisObject) > + return throwVMTypeError(globalObject, scope, "JSCallbackObject[Symbol.toPrimitive] method called on incompatible |this| value."_s); > + PreferredPrimitiveType hint = toPreferredPrimitiveType(globalObject, callFrame->argument(0)); As someone who has been here with JavaScriptCore and WebCore all along, quite sad that the coding styles are diverging on minor stylistic questions like how we use auto.
Committed r276660 (237087@main): <https://commits.webkit.org/237087@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 427040 [details].
<rdar://problem/77228749>