[WebIDL] Fix convertRecord() to throw on enumerable symbol |key|
Created attachment 423276 [details] Patch
Comment on attachment 423276 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=423276&action=review > Source/WebCore/bindings/js/JSDOMConvertStrings.cpp:38 > + VM& vm = lexicalGlobalObject.vm(); > + auto scope = DECLARE_THROW_SCOPE(vm); Is there any performance benefit to moving this inside the if statement? > Source/WebCore/bindings/js/JSDOMConvertStrings.cpp:103 > + if (!string) > + return { }; This seems like an unimportant optimization. I would leave it out; the stringToUSVString function can handle the null string without causing any problems. Then this can just be a one-liner. Also, we generally write string.isNull() instead of !string. Not sure why. return stringToUSVString(identifierToString(lexicalGlobalObject, identifier));
Created attachment 423404 [details] Patch Add UNLIKELY, move DECLARE_THROW_SCOPE into branch, and make identifierToUSVString() a one-liner.
(In reply to Darin Adler from comment #2) Thank you for reviewing convertRecord() changes! > Is there any performance benefit to moving this inside the if statement? I've found a few precedents of ThrowScope() created inside a branch, in hot inlined methods. This won't break validation of exception checks. Also, added an UNLIKELY, which is merely a compiler hint on how to emits jumps so common case is optimized. > This seems like an unimportant optimization. I would leave it out; the > stringToUSVString function can handle the null string without causing any > problems. Then this can just be a one-liner. Also, we generally write > string.isNull() instead of !string. Not sure why. > > return stringToUSVString(identifierToString(lexicalGlobalObject, > identifier)); Nice, fixed.
Committed r274561: <https://commits.webkit.org/r274561> All reviewed patches have been landed. Closing bug and clearing flags on attachment 423404 [details].
<rdar://problem/75528251>