Autogenerate passing union types as part of a functions variadic arguments
Created attachment 290620 [details] Patch
Created attachment 290627 [details] Patch
Created attachment 290628 [details] Patch
Created attachment 290636 [details] Patch
Created attachment 290639 [details] Patch
Created attachment 290655 [details] Patch
Comment on attachment 290655 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=290655&action=review > Source/WebCore/ChangeLog:12 > + * bindings/js/JSCharacterDataCustom.cpp: Removed. Fails to build on Windows: C:\cygwin\home\buildbot\WebKit\Source\WebCore\bindings\js\JSBindingsAllInOne.cpp(44): fatal error C1083: Cannot open include file: 'JSCharacterDataCustom.cpp': No such file or directory [C:\cygwin\home\buildbot\WebKit\WebKitBuild\Release\Source\WebCore\WebCore.vcxproj]
Comment on attachment 290655 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=290655&action=review > Source/WebCore/bindings/js/JSDOMConvert.h:79 > + static Optional<T> convertFailable(JSC::ExecState& state, JSC::JSValue value) I am not sure we need this, all convert() functions can already fail. > Source/WebCore/bindings/js/JSDOMConvert.h:129 > + static Optional<Ref<T>> convertFailable(JSC::ExecState& state, JSC::JSValue value) Why do we need the concept of 'Failable"? We previously converted interface types using convertWrapperType(). The way we check for failure is usually by checking if an exception was thrown via "state".
Comment on attachment 290655 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=290655&action=review Looks great otherwise. > Source/WebCore/bindings/scripts/CodeGenerator.pm:637 > + push @flattenedMemberTypes, $object->GetFlattenedMemberTypes($memberType) I think we usually use the version with the parentheses in this script: push(). > Source/WebCore/bindings/scripts/CodeGenerator.pm:639 > + push @flattenedMemberTypes, $memberType; ditto.
(In reply to comment #8) > Comment on attachment 290655 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=290655&action=review > > > Source/WebCore/bindings/js/JSDOMConvert.h:79 > > + static Optional<T> convertFailable(JSC::ExecState& state, JSC::JSValue value) > > I am not sure we need this, all convert() functions can already fail. > > > Source/WebCore/bindings/js/JSDOMConvert.h:129 > > + static Optional<Ref<T>> convertFailable(JSC::ExecState& state, JSC::JSValue value) > > Why do we need the concept of 'Failable"? > > We previously converted interface types using convertWrapperType(). The way > we check for failure is usually by checking if an exception was thrown via > "state". The issue is that I need to be able to return a Ref<T> from the convert function, since that is what the implementation files want in this case, but in the case that they fail, I cannot return a Ref<T>. So, returning an Optional<Ref<T>> is the way to go, and if I am doing that, I need to make it generic, so all converts must return an Optional<Foo>. Another way to go would be to have the convert function return a RefPtr<T> and have another static function on Converter that can transform the RefPtr<T> into a Ref<T> if there was no exception (and for non interface types, just pass the result through). Do you have a preference for either?
(In reply to comment #10) > (In reply to comment #8) > > Comment on attachment 290655 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=290655&action=review > > > > > Source/WebCore/bindings/js/JSDOMConvert.h:79 > > > + static Optional<T> convertFailable(JSC::ExecState& state, JSC::JSValue value) > > > > I am not sure we need this, all convert() functions can already fail. > > > > > Source/WebCore/bindings/js/JSDOMConvert.h:129 > > > + static Optional<Ref<T>> convertFailable(JSC::ExecState& state, JSC::JSValue value) > > > > Why do we need the concept of 'Failable"? > > > > We previously converted interface types using convertWrapperType(). The way > > we check for failure is usually by checking if an exception was thrown via > > "state". > > The issue is that I need to be able to return a Ref<T> from the convert > function, since that is what the implementation files want in this case, but > in the case that they fail, I cannot return a Ref<T>. So, returning an > Optional<Ref<T>> is the way to go, and if I am doing that, I need to make it > generic, so all converts must return an Optional<Foo>. > > Another way to go would be to have the convert function return a RefPtr<T> > and have another static function on Converter that can transform the > RefPtr<T> into a Ref<T> if there was no exception (and for non interface > types, just pass the result through). Do you have a preference for either? My preference would be to rely on convertWrapperType() which returns a raw pointer. I don't think the implementation really needs a Ref<>. We can call convertWrapperType() with the right IsNullable parameter. Then the calling code just needs to check if it threw an exception. Why would the convert<> function ref the object? I understand that your call site currently eventually wants to ref it but in general we pass raw pointers (or C++ references when not nullable) to the implementation to avoid ref counting churn. We then let the implementation decide if it wants to ref it or not.
Created attachment 290739 [details] Patch
Comment on attachment 290739 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=290739&action=review > Source/WebCore/bindings/js/JSDOMConvert.h:132 > + static Optional<Ref<T>> convertFailable(JSC::ExecState& state, JSC::JSValue value) Still using convertFailable? All convert functions can already fail and there is already a convert function for wrapper types.
Created attachment 290852 [details] Patch
Comment on attachment 290739 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=290739&action=review r=me but I think you should consider the comment Chris made about convertFailable. > Source/WebCore/bindings/scripts/CodeGenerator.pm:666 > + if ($memberType->isNullable) { > + $count++; > + } I like one-liner if for this kind of thing in perl. > Source/WebCore/bindings/scripts/CodeGenerator.pm:670 > + if ($memberType->isUnion) { > + $count += $object->GetNumberOfNullableMemberTypes($memberType); > + } Ditto. > Source/WebCore/bindings/scripts/CodeGenerator.pm:683 > + if ($numberOfNullableMembers > 1) { > + assert("Union types must only have 0 or 1 nullable types."); > + } Ditto. > Source/WebCore/bindings/scripts/CodeGenerator.pm:689 > + if ($numberOfNullableMembers == 1) { > + push(@idlUnionMemberTypes, "IDLNull"); > + } Ditto.
Sorry, my review comments (trivial tiny things) showed up just as you posted your new patch.
Committed r206956: <http://trac.webkit.org/changeset/206956>