Closed
Bug 273467
Opened 21 years ago
Closed 16 years ago
-"-0x1" results in 1 instead of NaN
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
VERIFIED
FIXED
mozilla1.9.3a1
Tracking | Status | |
---|---|---|
status1.9.2 | --- | beta1-fixed |
People
(Reporter: nallen, Assigned: wesongathedeveloper)
References
Details
(Keywords: verified1.9.2, Whiteboard: [good first bug] fixed-in-tracemonkey)
Attachments
(4 files, 2 obsolete files)
50 bytes,
text/html
|
Details | |
1.10 KB,
patch
|
brendan
:
review+
|
Details | Diff | Splinter Review |
77 bytes,
text/html
|
Details | |
1.11 KB,
patch
|
brendan
:
review+
|
Details | Diff | Splinter Review |
This evaluates as the negation of ToNumber("-0x01"). The grammar for ToNumber
does not permit hex integer literals to be signed. Any string that can't be
interpreted by the grammar is supposed to result in a NaN. IE follows the spec
and gives NaN.
This is difference between ToNumber and parseInt, which permits negative hex
literals.
Reporter | ||
Comment 1•21 years ago
|
||
Updated•16 years ago
|
Whiteboard: [good first bug]
Assignee | ||
Comment 3•16 years ago
|
||
Handle the above specified edge case
Assignee: general → wesongathedeveloper
Status: NEW → ASSIGNED
Attachment #393466 -
Flags: review?(brendan)
Comment 4•16 years ago
|
||
Comment on attachment 393466 [details] [diff] [review]
Patch v1
>diff --git a/js/src/jsnum.cpp b/js/src/jsnum.cpp
>--- a/js/src/jsnum.cpp
>+++ b/js/src/jsnum.cpp
>@@ -883,16 +883,27 @@ js_ValueToNumber(JSContext *cx, jsval *v
> /*
> * Note that ECMA doesn't treat a string beginning with a '0' as
> * an octal number here. This works because all such numbers will
> * be interpreted as decimal by js_strtod and will never get
> * passed to js_strtointeger (which would interpret them as
> * octal).
> */
> JSSTRING_CHARS_AND_END(str, bp, end);
You are patching old source. Please pull from hg.mozilla.org/mozilla-central.
>+
>+ // ECMA doesn't allow negative HEX numbers (bug 273467)
Nits: hex not HEX (it's not an acronym), and full-stop punctuation (.) at end of comment.
>+ const jschar *s;
This variable isn't necessary, see further below.
>+ JSBool negative;
>+
>+ s = js_SkipWhiteSpace(bp, end);
>+ if (s != end && (negative = (*s == '-')) != 0 &&
>+ s + 1 != end && s[1] == '0' &&
>+ s + 2 != end && (s[2] == 'X' || s[2] == 'x'))
>+ break;
There's no need for the single-use negative variable, nor for != 0 with a boolean.
Don't test redundantly: in order for a negative hex literal at least three characters must lie after the -, 0xN where N is a hex digit. So just test s + 3 < end and then match characters.
The break; is overindented by 3 columns.
The multiline condition should force bracing of the single-line break; consquent. See the style guide (https://wiki.mozilla.org/JavaScript:SpiderMonkey:C_Coding_Style).
>+
> if ((!js_strtod(cx, bp, end, &ep, &d) ||
> js_SkipWhiteSpace(ep, end) != end) &&
> (!js_strtointeger(cx, bp, end, &ep, 0, &d) ||
> js_SkipWhiteSpace(ep, end) != end)) {
No point in starting at bp here now that s has been advanced. OTOH, no point in const jschar *s given bp. May as well have just bp and not add s.
> break;
> }
Note style to match here, indentation and bracing.
/be
Assignee | ||
Comment 5•16 years ago
|
||
Isn't checking for bp + 2 < end sufficient (instead of bp + 3 < end) since no valid number can begin with -0x according to the grammar. This would also avoid the need to further match hex digits
Attachment #393466 -
Attachment is obsolete: true
Attachment #393966 -
Flags: review?(brendan)
Attachment #393466 -
Flags: review?(brendan)
Comment 6•16 years ago
|
||
Comment on attachment 393966 [details] [diff] [review]
Patch v2
>diff --git a/js/src/jsnum.cpp b/js/src/jsnum.cpp
>--- a/js/src/jsnum.cpp
>+++ b/js/src/jsnum.cpp
>@@ -919,16 +919,24 @@ js_ValueToNumber(JSContext *cx, jsval *v
> /*
> * Note that ECMA doesn't treat a string beginning with a '0' as
> * an octal number here. This works because all such numbers will
> * be interpreted as decimal by js_strtod and will never get
> * passed to js_strtointeger (which would interpret them as
> * octal).
> */
> str->getCharsAndEnd(bp, end);
>+
>+ /* ECMA doesn't allow negative hex numbers (bug 273467). */
>+ bp = js_SkipWhiteSpace(bp, end);
>+ if ((bp + 2 < end) && (*bp == '-') && (bp[1] == '0') &&
>+ (bp[2] == 'X' || bp[2] == 'x')) {
>+ break;
>+ }
>+
Great -- only nit: don't overparenthesize equality or relational ops against && and ||. Whoever lands this, please fix it (new patch fixing it is helpful, gets carry-over r+).
/be
Attachment #393966 -
Flags: review?(brendan) → review+
Updated•16 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 7•16 years ago
|
||
-"+0x01" should be NaN as well
Assignee | ||
Comment 8•16 years ago
|
||
Not sure if this should be filed as another bug, but requesting review again for change to handle + sign as well - see additional test case.
Attachment #394450 -
Flags: review?(brendan)
Assignee | ||
Comment 9•16 years ago
|
||
Fixed comment in Patch v3
Attachment #394450 -
Attachment is obsolete: true
Attachment #394490 -
Flags: review?(brendan)
Attachment #394450 -
Flags: review?(brendan)
Updated•16 years ago
|
Attachment #394490 -
Flags: review?(brendan) → review+
Comment 10•16 years ago
|
||
Comment on attachment 394490 [details] [diff] [review]
Patch v4
Good catch, I often forget about unary + as operator or in string-to-number. Patch looks great to land as is, thanks!
/be
Comment 11•16 years ago
|
||
And here it is:
http://hg.mozilla.org/tracemonkey/rev/a2ab5097c5e5
(Note: This bug will be marked RESOLVED FIXED in a few days, once the change finds its way into the mozilla-central repository.)
Keywords: checkin-needed
Whiteboard: [good first bug] → [good first bug] fixed-in-tracemonkey
Comment 12•16 years ago
|
||
http://hg.mozilla.org/tracemonkey/rev/fdcb3fdf8ec9
js/tests/ecma/TypeConversion/9.3.1-3.js updated.
Flags: in-testsuite+
Comment 13•16 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Comment 14•16 years ago
|
||
should status1.9.2 be beta1-fixed?
Comment 15•16 years ago
|
||
Comment 16•16 years ago
|
||
status1.9.2:
--- → beta1-fixed
Flags: wanted1.9.2+
Comment 17•16 years ago
|
||
Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.3a1pre) Gecko/20091008 Minefield/3.7a1pre
Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.2b1pre) Gecko/20091008 Namoroka/3.6b1pre
V. Fixed based on the first testcase. I get NaN now.
Status: RESOLVED → VERIFIED
Keywords: verified1.9.2
Updated•16 years ago
|
Target Milestone: --- → mozilla1.9.3a1
You need to log in
before you can comment on or make changes to this bug.
Description
•