Introducing construct ability into JS executables
Let's decouple the construct ability from the builtin functions. Currently, builtin functions are not constructor by this patch http://trac.webkit.org/changeset/182995. In this patch, when the given function is builtin JS function, we recognize it as the non-constructor function. But, we need to relax it to implement some constructors in builtins JS. By decoupling the construct ability from whether the function is builtin or not, we can provide 1. constructors written in builtin JS 2. non-constructors in normal JS functions (1) is needed for Promise constructor. And (2) is needed for method functions and arrow functions.
http://www.ecma-international.org/ecma-262/6.0/#sec-functioncreate If kind is not Normal, let allocKind be "non-constructor".
Created attachment 257243 [details] Patch WIP, the test forthcomming
I've asked for the clarification in static MethodDefinition and construct ability. https://esdiscuss.org/topic/propertydefinitionevaluation-for-static-methoddefinition
Created attachment 257280 [details] Patch
Comment on attachment 257280 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=257280&action=review r- as i don't like the subtlety of the current behaviour > Source/JavaScriptCore/ChangeLog:26 > + Currenctly, we make the builtin function the constructor when the function name is capitalized. I think that's kind of icky -- something we consider doing that i think would be clearer would be to allow builtins defined as constructor thing(...) { ... } This would actually allow us (in a later patch) to use builtins to more cleanly define those functions that have distinct construct vs. call behaviours, e.g constructor thing(...) {} function thing( ... ) {} would register the distinct executables for the construct vs. call behavior of "thing"
Comment on attachment 257280 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=257280&action=review Thank you for your review. >> Source/JavaScriptCore/ChangeLog:26 >> + Currenctly, we make the builtin function the constructor when the function name is capitalized. > > I think that's kind of icky -- something we consider doing that i think would be clearer would be to allow builtins defined as > > constructor thing(...) { ... } > > This would actually allow us (in a later patch) to use builtins to more cleanly define those functions that have distinct construct vs. call behaviours, e.g > > constructor thing(...) {} > function thing( ... ) {} > > would register the distinct executables for the construct vs. call behavior of "thing" That sounds very nice to me. I'll upload the revised patch.
Created attachment 257294 [details] Patch
Comment on attachment 257294 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=257294&action=review r=me with some suggested fixups below. > Source/JavaScriptCore/builtins/BuiltinExecutables.cpp:63 > +UnlinkedFunctionExecutable* BuiltinExecutables::createExecutableInternal(const SourceCode& source, const Identifier& name, ConstructorKind constructorKind, bool hasConstruct) Let's use the enum type (ConstructAbility) for the function argument instead of a bool. Bool is not nice as an argument type because it is often difficult to understand what "f(false)" or "f(true)" means just by reading it. > Source/JavaScriptCore/builtins/BuiltinExecutables.h:64 > - UnlinkedFunctionExecutable* createBuiltinExecutable(const SourceCode& code, const Identifier& name) > + UnlinkedFunctionExecutable* createBuiltinExecutable(const SourceCode& code, const Identifier& name, bool hasConstruct) > { > - return createExecutableInternal(code, name, ConstructorKind::None); > + return createExecutableInternal(code, name, ConstructorKind::None, hasConstruct); > } > - UnlinkedFunctionExecutable* createExecutableInternal(const SourceCode&, const Identifier&, ConstructorKind); > + UnlinkedFunctionExecutable* createExecutableInternal(const SourceCode&, const Identifier&, ConstructorKind, bool hasConstruct); Ditto: Let's use the enum type instead of bool here. > Source/JavaScriptCore/bytecode/UnlinkedCodeBlock.cpp:104 > + , m_hasConstruct(constructAbility == ConstructAbility::HasConstruct) Let's store the enum type instead of a bool. > Source/JavaScriptCore/bytecode/UnlinkedCodeBlock.h:103 > + HasConstruct, > + NoConstruct, Let's call these "CanConstruct" and "CannotConstruct".
Comment on attachment 257294 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=257294&action=review Thank you for your review and suggestions. >> Source/JavaScriptCore/builtins/BuiltinExecutables.cpp:63 >> +UnlinkedFunctionExecutable* BuiltinExecutables::createExecutableInternal(const SourceCode& source, const Identifier& name, ConstructorKind constructorKind, bool hasConstruct) > > Let's use the enum type (ConstructAbility) for the function argument instead of a bool. Bool is not nice as an argument type because it is often difficult to understand what "f(false)" or "f(true)" means just by reading it. OK, so I'll fix it by, 1. Create the ConstructAbility.h 2. Include the header in JSCBuiltins.cpp generated by the script 3. Use it >> Source/JavaScriptCore/builtins/BuiltinExecutables.h:64 >> + UnlinkedFunctionExecutable* createExecutableInternal(const SourceCode&, const Identifier&, ConstructorKind, bool hasConstruct); > > Ditto: Let's use the enum type instead of bool here. Fixed. >> Source/JavaScriptCore/bytecode/UnlinkedCodeBlock.cpp:104 >> + , m_hasConstruct(constructAbility == ConstructAbility::HasConstruct) > > Let's store the enum type instead of a bool. Here, due to the object size limitation, we used m_hasConstruct with the bit field (width 1). This is because the other fields also use the bit field to reduce the UnlinkedFunctionExecutable size. Just storing the enum type enlarges the object size... So, instead of storing the enum type, storing the bit and add the getter that converts the boolean to the enum value. >> Source/JavaScriptCore/bytecode/UnlinkedCodeBlock.h:103 >> + NoConstruct, > > Let's call these "CanConstruct" and "CannotConstruct". Thanks. Fixed.
Comment on attachment 257294 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=257294&action=review >>> Source/JavaScriptCore/bytecode/UnlinkedCodeBlock.cpp:104 >>> + , m_hasConstruct(constructAbility == ConstructAbility::HasConstruct) >> >> Let's store the enum type instead of a bool. > > Here, due to the object size limitation, we used m_hasConstruct with the bit field (width 1). This is because the other fields also use the bit field to reduce the UnlinkedFunctionExecutable size. > Just storing the enum type enlarges the object size... > So, instead of storing the enum type, storing the bit and add the getter that converts the boolean to the enum value. you can set an enum to be stored as a bitfield :D
Comment on attachment 257294 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=257294&action=review >>>> Source/JavaScriptCore/bytecode/UnlinkedCodeBlock.cpp:104 >>>> + , m_hasConstruct(constructAbility == ConstructAbility::HasConstruct) >>> >>> Let's store the enum type instead of a bool. >> >> Here, due to the object size limitation, we used m_hasConstruct with the bit field (width 1). This is because the other fields also use the bit field to reduce the UnlinkedFunctionExecutable size. >> Just storing the enum type enlarges the object size... >> So, instead of storing the enum type, storing the bit and add the getter that converts the boolean to the enum value. > > you can set an enum to be stored as a bitfield :D That's fine! So just storing the enum type directly with the bitfield.
Comment on attachment 257294 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=257294&action=review >>>>>> Source/JavaScriptCore/bytecode/UnlinkedCodeBlock.cpp:104 >>>>>> + , m_hasConstruct(constructAbility == ConstructAbility::HasConstruct) >>>>> >>>>> Let's store the enum type instead of a bool. >>>> >>>> Here, due to the object size limitation, we used m_hasConstruct with the bit field (width 1). This is because the other fields also use the bit field to reduce the UnlinkedFunctionExecutable size. >>>> Just storing the enum type enlarges the object size... >>>> So, instead of storing the enum type, storing the bit and add the getter that converts the boolean to the enum value. >>> >>> you can set an enum to be stored as a bitfield :D >> >> That's fine! So just storing the enum type directly with the bitfield. > > That's fine! So just storing the enum type directly with the bitfield. Oops, warned by the style checker, "Please declare enum bitfields as unsigned integral types." So just follow the other accessors (like FunctionMode, ConstructorKind etc.), storing enum directly to the bitfield (unsigned) and retrieve it by static_cast in the accessor.
Committed r187205: <http://trac.webkit.org/changeset/187205>