Bug 109902 – [JSC] MAYBE_MISSING_PARAMETER(..., DefaultIsNullString) macro is redundant

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore JavaScript

[Kentaro Hara]

c.f. corresponding V8 bug: https://bugs.webkit.org/show_bug.cgi?id=109899

Currently:

- MAYBE_MISSING_PARAMETER(exec, index, DefaultIsUndefined) returns exec->argument(index).
- MAYBE_MISSING_PARAMETER(exec, index, DefaultIsNullString) returns JSValue() if exec->argument(index) is missing (i.e. the length of the argument is less than index). It returns exec->argument(index) otherwise.

No one other than CodeGeneratorJS.pm uses MAYBE_MISSING_PARAMETER(exec, index, DefaultIsUndefined). Instead, we simply use exec->argument(index). We should remove the redundant usage in CodeGeneratorJS.pm too. The long-name macro has been making generated code less readable.

In addition, we can rename MAYBE_MISSING_PARAMETER() to MissingIsNullString().

[Kentaro Hara]

Created attachment 188494 [details]
Patch

[Adam Barth]

Comment on attachment 188494 [details]
Patch

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

> Source/WebCore/bindings/js/JSDOMBinding.h:62
> +#define MissingIsNullString(exec, index) (((index) >= (exec)->argumentCount()) ? (JSValue()) : ((exec)->argument(index)))

Same comment.  This should probably be an inline function.  If it can't be an inline function, we should keep the ALL_CAPS name.

[Darin Adler]

Comment on attachment 188494 [details]
Patch

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

>> Source/WebCore/bindings/js/JSDOMBinding.h:62
>> +#define MissingIsNullString(exec, index) (((index) >= (exec)->argumentCount()) ? (JSValue()) : ((exec)->argument(index)))
> 
> Same comment.  This should probably be an inline function.  If it can't be an inline function, we should keep the ALL_CAPS name.

Further, both the names MAYBE_MISSING_PARAMETER and MissingIsNullString are examples of the kind of confusing names we try to avoid. A function should be named after its return value. And even though it’s used to implement DefaultIsNullString, this does not return a string, so it’s really confusing to mention string in its name.

ARGUMENT_OR_NULL (or ArgumentOrNull if it can be an inline function) would be one possible name. Not great, but better than the two we used in the past.

[Kentaro Hara]

Created attachment 189002 [details]
patch for landing

[Kentaro Hara]

(In reply to comment #3)
> (From update of attachment 188494 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=188494&action=review
> 
> >> Source/WebCore/bindings/js/JSDOMBinding.h:62
> >> +#define MissingIsNullString(exec, index) (((index) >= (exec)->argumentCount()) ? (JSValue()) : ((exec)->argument(index)))
> > 
> > Same comment.  This should probably be an inline function.  If it can't be an inline function, we should keep the ALL_CAPS name.
> 
> Further, both the names MAYBE_MISSING_PARAMETER and MissingIsNullString are examples of the kind of confusing names we try to avoid. A function should be named after its return value. And even though it’s used to implement DefaultIsNullString, this does not return a string, so it’s really confusing to mention string in its name.
> 
> ARGUMENT_OR_NULL (or ArgumentOrNull if it can be an inline function) would be one possible name. Not great, but better than the two we used in the past.

Thanks, changed it to an inline function and renamed it to argumentOrNull().

[WebKit Review Bot]

Comment on attachment 189002 [details]
patch for landing

Clearing flags on attachment: 189002

Committed r143304: <http://trac.webkit.org/changeset/143304>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Julien Brianceau]

This patch breaks our SH4 build slave: http://build.webkit.org/builders/Qt%20Linux%20SH4%20Release/builds/20416

[Kentaro Hara]

(In reply to comment #8)
> This patch breaks our SH4 build slave: http://build.webkit.org/builders/Qt%20Linux%20SH4%20Release/builds/20416

Sorry, looking.

[Kentaro Hara]

Fixed the build error in r143306.

[Julien Brianceau]

(In reply to comment #10)
> Fixed the build error in r143306.
Thanks a lot !

[Ryosuke Niwa]

Landed another build fix in r143309.

[Csaba Osztrogonác]

Comment on attachment 189002 [details]
patch for landing

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

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2758
> +            push(@$outputArray, "    " . GetNativeTypeFromSignature($parameter) . " $name(" . JSValueToNative($parameter, $optional eq "DefaultIsNullString" ? "argumentOrNull(exec, $argsIndex)" : "exec->argument($argsIndex)") . ");\n");

It caused the following warning:
Use of uninitialized value $optional in string eq at /home/oszi/WebKit/Source/WebCore/bindings/scripts/CodeGeneratorJS.pm line 2758.

Could you check and fix it, please?

[Csaba Osztrogonác]

(In reply to comment #13)
> (From update of attachment 189002 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=189002&action=review
> 
> > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2758
> > +            push(@$outputArray, "    " . GetNativeTypeFromSignature($parameter) . " $name(" . JSValueToNative($parameter, $optional eq "DefaultIsNullString" ? "argumentOrNull(exec, $argsIndex)" : "exec->argument($argsIndex)") . ");\n");
> 
> It caused the following warning:
> Use of uninitialized value $optional in string eq at /home/oszi/WebKit/Source/WebCore/bindings/scripts/CodeGeneratorJS.pm line 2758.
> 
> Could you check and fix it, please?

See the Qt bot for details:
http://build.webkit.org/builders/Qt%20Linux%20Release/builds/57635/steps/compile-webkit/logs/stdio

[Csaba Osztrogonác]

(In reply to comment #14)
> (In reply to comment #13)
> > (From update of attachment 189002 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=189002&action=review
> > 
> > > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2758
> > > +            push(@$outputArray, "    " . GetNativeTypeFromSignature($parameter) . " $name(" . JSValueToNative($parameter, $optional eq "DefaultIsNullString" ? "argumentOrNull(exec, $argsIndex)" : "exec->argument($argsIndex)") . ");\n");
> > 
> > It caused the following warning:
> > Use of uninitialized value $optional in string eq at /home/oszi/WebKit/Source/WebCore/bindings/scripts/CodeGeneratorJS.pm line 2758.
> > 
> > Could you check and fix it, please?
> 
> See the Qt bot for details:
> http://build.webkit.org/builders/Qt%20Linux%20Release/builds/57635/steps/compile-webkit/logs/stdio

Fixed by https://trac.webkit.org/changeset/143308

[Csaba Osztrogonác]

Additionally it broke the bindings tests on all bot. Could you fix it?

[Csaba Osztrogonác]

(In reply to comment #16)
> Additionally it broke the bindings tests on all bot. Could you fix it?

ping for fix?

[Kentaro Hara]

(In reply to comment #17)
> (In reply to comment #16)
> > Additionally it broke the bindings tests on all bot. Could you fix it?
> 
> ping for fix?

Fixed in r143374. Sorry for late.