Bug 34575 – [Qt] QScriptValue::isNumber() returns an incorrect value

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Jędrzej Nowacki]

For these values:
QScriptValue v1(0, 0.0);
QScriptValue v2(0.0);
QScriptValue v3(0, 0);
QScriptValue v4(0);
::isNumber() returns false which is incorrect.

[Jędrzej Nowacki]

Created attachment 48135 [details]
Fix v1

[WebKit Review Bot]

Attachment 48135 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
Ignoring "JavaScriptCore/qt/api/qscriptvalue_p.h": this file is exempt from the style guide.
JavaScriptCore/qt/tests/qscriptvalue/tst_qscriptvalue.h:58:  isNumber_data is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/qt/tests/qscriptvalue/tst_qscriptvalue.h:100:  isNumber_initData is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/qt/tests/qscriptvalue/tst_qscriptvalue.h:101:  isNumber_makeData is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/qt/tests/qscriptvalue/tst_qscriptvalue.h:102:  isNumber_test is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 4


If any of these errors are false positives, please file a bug against check-webkit-style.

[Eric Seidel (no email)]

Comment on attachment 48135 [details]
Fix v1

So previously 0 and NaN were not "numbers"?  (assuming m_number is a float here).  Why?

WebKit style is not to name arguments when their names add no value:
 102     void isNumber_test(const char* expr, const QScriptValue& value);

"value" there adds no value.

I don't understand how this actually adds the test data:
 371     newRow(expr) << isNumber.contains(expr);
but I think I'm simply not familiar enough with Qt testing infrastructure.

Looks like you'll need a Qt reviewer here.

[Jędrzej Nowacki]

Created attachment 48399 [details]
Fix v2

[Jędrzej Nowacki]

(In reply to comment #3)
> (From update of attachment 48135 [details])
> So previously 0 and NaN were not "numbers"?  (assuming m_number is a float
> here).  Why?
m_number is double and it was casted automatically to bool. (bool)0.0 and (bool)NaN is false. 

> WebKit style is not to name arguments when their names add no value:
>  102     void isNumber_test(const char* expr, const QScriptValue& value);
> 
> "value" there adds no value.
fixed.

> I don't understand how this actually adds the test data:
>  371     newRow(expr) << isNumber.contains(expr);
> but I think I'm simply not familiar enough with Qt testing infrastructure.

Essentially, isNumber() method is called for each QScriptValue created in the initScriptValues() so it is about 100 different values. The line you mentioned provides expected results only which are passed to the isNumber_test() by QFETCH makro.

[WebKit Commit Bot]

Comment on attachment 48399 [details]
Fix v2

Clearing flags on attachment: 48399

Committed r54577: <http://trac.webkit.org/changeset/54577>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.