Closed Bug 273467 Opened 21 years ago Closed 16 years ago

-"-0x1" results in 1 instead of NaN

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
minor

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)

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.
Attached file the obvious test case
-> default qa
QA Contact: pschwartau → general
Whiteboard: [good first bug]
Attached patch Patch v1 (obsolete) — Splinter Review
Handle the above specified edge case
Assignee: general → wesongathedeveloper
Status: NEW → ASSIGNED
Attachment #393466 - Flags: review?(brendan)
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
Attached patch Patch v2Splinter Review
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 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+
Attached file Additonal Testcase
-"+0x01" should be NaN as well
Attached patch Patch v3 (obsolete) — Splinter Review
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)
Attached patch Patch v4Splinter Review
Fixed comment in Patch v3
Attachment #394450 - Attachment is obsolete: true
Attachment #394490 - Flags: review?(brendan)
Attachment #394450 - Flags: review?(brendan)
Attachment #394490 - Flags: review?(brendan) → review+
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
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
Depends on: 510692
http://hg.mozilla.org/tracemonkey/rev/fdcb3fdf8ec9 js/tests/ecma/TypeConversion/9.3.1-3.js updated.
Flags: in-testsuite+
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
should status1.9.2 be beta1-fixed?
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
Target Milestone: --- → mozilla1.9.3a1
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: