Bug 34575 - [Qt] QScriptValue::isNumber() returns an incorrect value
Summary: [Qt] QScriptValue::isNumber() returns an incorrect value
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Jędrzej Nowacki
URL:
Keywords: Qt
Depends on: 34533
Blocks: 31863
  Show dependency treegraph
 
Reported: 2010-02-04 05:51 PST by Jędrzej Nowacki
Modified: 2010-02-09 18:19 PST (History)
3 users (show)

See Also:


Attachments
Fix v1 (5.39 KB, patch)
2010-02-04 05:58 PST, Jędrzej Nowacki
no flags Details | Formatted Diff | Diff
Fix v2 (5.38 KB, patch)
2010-02-09 02:49 PST, Jędrzej Nowacki
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jędrzej Nowacki 2010-02-04 05:51:06 PST
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.
Comment 1 Jędrzej Nowacki 2010-02-04 05:58:35 PST
Created attachment 48135 [details]
Fix v1
Comment 2 WebKit Review Bot 2010-02-04 06:03:15 PST
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.
Comment 3 Eric Seidel (no email) 2010-02-04 15:34:23 PST
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.
Comment 4 Jędrzej Nowacki 2010-02-09 02:49:28 PST
Created attachment 48399 [details]
Fix v2
Comment 5 Jędrzej Nowacki 2010-02-09 03:49:09 PST
(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.
Comment 6 WebKit Commit Bot 2010-02-09 18:19:35 PST
Comment on attachment 48399 [details]
Fix v2

Clearing flags on attachment: 48399

Committed r54577: <http://trac.webkit.org/changeset/54577>
Comment 7 WebKit Commit Bot 2010-02-09 18:19:40 PST
All reviewed patches have been landed.  Closing bug.