Summary: | Use the reject() helper function for conditionally throwing TypeErrors. | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Mark Lam <mark.lam> | ||||
Component: | JavaScriptCore | Assignee: | Mark Lam <mark.lam> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | alecflett, beidson, commit-queue, fpizlo, ggaren, jfbastien, jsbell, keith_miller, msaboff, saam, ysuzuki | ||||
Priority: | P2 | ||||||
Version: | WebKit Local Build | ||||||
Hardware: | Unspecified | ||||||
OS: | Unspecified | ||||||
Bug Depends on: | |||||||
Bug Blocks: | 163446 | ||||||
Attachments: |
|
Description
Mark Lam
2016-10-15 15:13:13 PDT
Created attachment 291728 [details]
proposed patch.
Comment on attachment 291728 [details]
proposed patch.
I don’t understand why a function that throws an error object is named "reject".
(In reply to comment #2) > Comment on attachment 291728 [details] > proposed patch. > > I don’t understand why a function that throws an error object is named > "reject". I didn't coin the name. The name came from https://trac.webkit.org/changeset/104488. The comment attached to the addition of the reject function states "Helper for the DefineOwnProperty? algorithm." The code which uses it in JSArray::defineOwnNumericProperty() (from https://trac.webkit.org/changeset/104488/trunk/Source/JavaScriptCore/runtime/JSArray.cpp) has this comment: // 3. If current is undefined and extensible is false, then Reject. My guess is that Reject was a term used in the ECMAScript spec back then. I no longer see the term used in that way in the current spec. What the function really does is: failAndThrowTypeErrorIfNeeded(). I'm not sure that that's the best name for it either. Would you be ok with me moving forward with this patch as is, and renaming the function to a better name in a subsequent patch? This patch has passed the JSC and layout tests. Comment on attachment 291728 [details] proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=291728&action=review Why change many of the calls to explicitly use ASCIILiteral instead of doing it implicitly? The code was inconsistent before, is the implicit one not WebKit style? (follow up: why is that a supported thing) > Source/JavaScriptCore/runtime/JSArray.cpp:217 > + return reject(exec, scope, throwException, ASCIILiteral("Attempting to define numeric property on array with non-writable length property.")); Why is this string inline? Others above are changed to a predefined thing. Some others bellow are also inline char*. (In reply to comment #5) > Comment on attachment 291728 [details] > proposed patch. > > View in context: > https://bugs.webkit.org/attachment.cgi?id=291728&action=review > > Why change many of the calls to explicitly use ASCIILiteral instead of doing > it implicitly? The code was inconsistent before, is the implicit one not > WebKit style? (follow up: why is that a supported thing) This is a performance thing. Here's why: 1. The ASCIILiteral(const char* characters) constructor is explicit. 2. The String(const char* characters) constructor can be implicit. 3. The String(const char* characters) constructor will malloc and copy over the string. The String(ASCIILiteral characters) constructor will NOT malloc and copy the string. 4. throwTypeError() takes either a ASCIILiteral or a const String&. Hence, explicitly using the ASCIILiteral saves us unnecessary mallocs and copies. > > Source/JavaScriptCore/runtime/JSArray.cpp:217 > > + return reject(exec, scope, throwException, ASCIILiteral("Attempting to define numeric property on array with non-writable length property.")); > > Why is this string inline? Others above are changed to a predefined thing. > Some others bellow are also inline char*. Because there's only one instance of this string AFAIK. Hence, it's not worth the effort to declare it elsewhere. (In reply to comment #6) > (In reply to comment #5) > > Comment on attachment 291728 [details] > > proposed patch. > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=291728&action=review > > > > Why change many of the calls to explicitly use ASCIILiteral instead of doing > > it implicitly? The code was inconsistent before, is the implicit one not > > WebKit style? (follow up: why is that a supported thing) > > This is a performance thing. Here's why: > 1. The ASCIILiteral(const char* characters) constructor is explicit. > 2. The String(const char* characters) constructor can be implicit. > 3. The String(const char* characters) constructor will malloc and copy over > the string. > The String(ASCIILiteral characters) constructor will NOT malloc and copy > the string. > 4. throwTypeError() takes either a ASCIILiteral or a const String&. > > Hence, explicitly using the ASCIILiteral saves us unnecessary mallocs and > copies. Gotcha, thanks for explaining. It's a bit surprising that there's a perf gotcha hiding in the API, but I'm guessing the String version of these is also useful so it can't just get removed (and an explicit char* -> String is a PITA so it shouldn't be changed either). > > > Source/JavaScriptCore/runtime/JSArray.cpp:217 > > > + return reject(exec, scope, throwException, ASCIILiteral("Attempting to define numeric property on array with non-writable length property.")); > > > > Why is this string inline? Others above are changed to a predefined thing. > > Some others bellow are also inline char*. > > Because there's only one instance of this string AFAIK. Hence, it's not > worth the effort to declare it elsewhere. Ah, makes sense. Ideally the non-unique ones get COMDAT'd away as well, but that requires LTO, so manual COMDAT is sadly / probably safer. Agreed with the comment on "reject" (only just saw it), probably worth changing separately. Happy to do so if you want. lgtm with all the power my non-reviewer self has :-) Comment on attachment 291728 [details] proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=291728&action=review Nice. >>>> Source/JavaScriptCore/runtime/JSArray.cpp:217 >>>> + return reject(exec, scope, throwException, ASCIILiteral("Attempting to define numeric property on array with non-writable length property.")); >>> >>> Why is this string inline? Others above are changed to a predefined thing. Some others bellow are also inline char*. >> >> Because there's only one instance of this string AFAIK. Hence, it's not worth the effort to declare it elsewhere. > > Ah, makes sense. Ideally the non-unique ones get COMDAT'd away as well, but that requires LTO, so manual COMDAT is sadly / probably safer. > > > Agreed with the comment on "reject" (only just saw it), probably worth changing separately. Happy to do so if you want. > > > lgtm with all the power my non-reviewer self has :-) I agree with the policy that used-once strings should be inline. > Source/JavaScriptCore/runtime/JSFunction.cpp:548 > + if (descriptor.configurablePresent() && descriptor.configurable()) > + return reject(exec, scope, throwException, ASCIILiteral(UnconfigurablePropertyChangeConfigurabilityError)); > + if (descriptor.enumerablePresent() && descriptor.enumerable()) > + return reject(exec, scope, throwException, ASCIILiteral(UnconfigurablePropertyChangeEnumerabilityError)); > + if (descriptor.isAccessorDescriptor()) > + return reject(exec, scope, throwException, ASCIILiteral(UnconfigurablePropertyChangeAccessMechanismError)); > + if (descriptor.writablePresent() && descriptor.writable()) > + return reject(exec, scope, throwException, ASCIILiteral(UnconfigurablePropertyChangeWritabilityError)); > + if (!valueCheck) > + return reject(exec, scope, throwException, ASCIILiteral(ReadonlyPropertyChangeError)); Wow, this is so much cleaner! Comment on attachment 291728 [details]
proposed patch.
I'll commit this manually.
Thanks for the review. Landed in r207411: <http://trac.webkit.org/r207411>. FYI, the bug to rename reject() (since Darin pointed out that reject is a weak name for it) is https://bugs.webkit.org/show_bug.cgi?id=163549. |