Drop Dictionary from CanUseWTFOptionalForParameter()

• Component: Bindings
• Status: RESOLVED FIXED

[Chris Dumez]

As per Web IDL, optional dictionary parameters are always considered to have a default value of an empty dictionary, unless otherwise specified. There is therefore never any need to use Optional<> for it. Just implement this behavior in the bindings generator and drop blacklisting of Dictionary from CanUseWTFOptionalForParameter().

[Chris Dumez]

Created attachment 277332 [details]
Patch

[Chris Dumez]

Created attachment 277335 [details]
Patch

[Chris Dumez]

Comment on attachment 277335 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=277335&action=review

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:4279
>      }

Got the following error otherwise:
/Volumes/Data/EWS/WebKit/WebKitBuild/Release/DerivedSources/WebCore/JSEventSource.cpp:107:67: error: initializer list cannot be used on the right hand side of operator '?'
    Dictionary eventSourceInit = state->argument(1).isUndefined() ? Dictionary() : { state, state->uncheckedArgument(1) };
                                                                  ^                ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 error generated.

[Darin Adler]

Comment on attachment 277335 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=277335&action=review

>> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:4279
>>      }
> 
> Got the following error otherwise:
> /Volumes/Data/EWS/WebKit/WebKitBuild/Release/DerivedSources/WebCore/JSEventSource.cpp:107:67: error: initializer list cannot be used on the right hand side of operator '?'
>     Dictionary eventSourceInit = state->argument(1).isUndefined() ? Dictionary() : { state, state->uncheckedArgument(1) };
>                                                                   ^                ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 1 error generated.

OK, but this will conflict with my patch that replaces Dictionary with JSDictionary. No big deal I guess. I can wait for this to land and then rebase.

[Darin Adler]

Comment on attachment 277335 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=277335&action=review

> Source/WebCore/bindings/scripts/test/JS/JSTestObj.cpp:3726
> -    Dictionary ooo = { state, state->argument(1) };
> +    Dictionary ooo = state->argument(1).isUndefined() ? Dictionary() : Dictionary(state, state->uncheckedArgument(1));

We really don’t need this check for isUndefined at all. Dictionary will do this if it’s passed an undefined value. Clearing commit-queue so you can consider a version that takes advantage of that.

[Chris Dumez]

(In reply to comment #5)
> Comment on attachment 277335 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=277335&action=review
> 
> > Source/WebCore/bindings/scripts/test/JS/JSTestObj.cpp:3726
> > -    Dictionary ooo = { state, state->argument(1) };
> > +    Dictionary ooo = state->argument(1).isUndefined() ? Dictionary() : Dictionary(state, state->uncheckedArgument(1));
> 
> We really don’t need this check for isUndefined at all. Dictionary will do
> this if it’s passed an undefined value. Clearing commit-queue so you can
> consider a version that takes advantage of that.

I understand that. However, special casing some types like this does make the bindings generator a bit more complicated. Also note that even though it looks like I am adding one branch, I am also removing one by calling uncheckedArgument() instead of argument().

[Darin Adler]

Comment on attachment 277335 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=277335&action=review

>>> Source/WebCore/bindings/scripts/test/JS/JSTestObj.cpp:3726
>>> +    Dictionary ooo = state->argument(1).isUndefined() ? Dictionary() : Dictionary(state, state->uncheckedArgument(1));
>> 
>> We really don’t need this check for isUndefined at all. Dictionary will do this if it’s passed an undefined value. Clearing commit-queue so you can consider a version that takes advantage of that.
> 
> I understand that. However, special casing some types like this does make the bindings generator a bit more complicated. Also note that even though it looks like I am adding one branch, I am also removing one by calling uncheckedArgument() instead of argument().

Yes, agreed.

The point remains that we are generating redundant code here. Inside the Dictionary constructor is a second check for undefined, identical to the extra one we generated here. And all the work in the call to uncheckedArgument is redundant with part of what argument already did, although perhaps the compiler will optimize out the common subexpression. And also, the entire call to the second Dictionary constructor will make the code larger.

I think it’s good to make an optimization for a case like this even if the code generator ends up *slightly* more complicated. And the complication here is quite minor. We just need it to generate the same code it does for non-optional dictionaries. See the code a couple lines above.

[Chris Dumez]

(In reply to comment #7)
> Comment on attachment 277335 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=277335&action=review
> 
> >>> Source/WebCore/bindings/scripts/test/JS/JSTestObj.cpp:3726
> >>> +    Dictionary ooo = state->argument(1).isUndefined() ? Dictionary() : Dictionary(state, state->uncheckedArgument(1));
> >> 
> >> We really don’t need this check for isUndefined at all. Dictionary will do this if it’s passed an undefined value. Clearing commit-queue so you can consider a version that takes advantage of that.
> > 
> > I understand that. However, special casing some types like this does make the bindings generator a bit more complicated. Also note that even though it looks like I am adding one branch, I am also removing one by calling uncheckedArgument() instead of argument().
> 
> Yes, agreed.
> 
> The point remains that we are generating redundant code here. Inside the
> Dictionary constructor is a second check for undefined, identical to the
> extra one we generated here. And all the work in the call to
> uncheckedArgument is redundant with part of what argument already did,
> although perhaps the compiler will optimize out the common subexpression.
> And also, the entire call to the second Dictionary constructor will make the
> code larger.
> 
> I think it’s good to make an optimization for a case like this even if the
> code generator ends up *slightly* more complicated. And the complication
> here is quite minor. We just need it to generate the same code it does for
> non-optional dictionaries. See the code a couple lines above.

Ok, Fair enough. Dictionary is not the only type we unnecessarily check of undefined either. I'll rework the patch.

[Darin Adler]

(In reply to comment #8)
> Ok, Fair enough. Dictionary is not the only type we unnecessarily check of
> undefined either. I'll rework the patch.

I don’t think you need to do this immediately, may not be worth your time right now, but eventually it seems better to generate the more efficient code.

[Chris Dumez]

Created attachment 277393 [details]
Patch

[Chris Dumez]

Comment on attachment 277393 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=277393&action=review

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3392
> +sub WillConvertUndefinedToDefaultParameterValue

Darin, can you please take another look since I added the optimization you suggested?

[Darin Adler]

Comment on attachment 277393 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=277393&action=review

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:4310
> -        return "{ state, $value }";
> +        return "Dictionary(state, $value)";

I don’t think you need this change any more.

[Chris Dumez]

Created attachment 277395 [details]
Patch

[Chris Dumez]

Comment on attachment 277395 [details]
Patch

Yes, reverted that change.

[WebKit Commit Bot]

Comment on attachment 277395 [details]
Patch

Clearing flags on attachment: 277395

Committed r200099: <http://trac.webkit.org/changeset/200099>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.