uneval should use "(void 0)" instead of "undefined"

VERIFIED FIXED

Status

()

--
minor
VERIFIED FIXED
12 years ago
11 years ago

People

(Reporter: jruderman, Assigned: crowderbt)

Tracking

({testcase})

Trunk
x86
Mac OS X
testcase
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

12 years ago
js> uneval({a: undefined})
({a:undefined})

This is wrong because "undefined" is a mutable property of the global object.  In bug 351219, Brendan suggested using "(void 0)" instead.
(Assignee)

Comment 1

11 years ago
I wonder if this too should be WONTFIXed.  How much of the web would be broken by this change?
(Reporter)

Comment 2

11 years ago
How much of the web even uses uneval?  Only SpiderMonkey supports it.
(Assignee)

Comment 3

11 years ago
Created attachment 267471 [details] [diff] [review]
adds "(void 0)" atom and uses it for toString of JSVAL_VOID

Well, I think in our case toSource and toString are not so different (especially here).  This seems like a risky change, but here is what a patch would look like (I think) if we were to implement it.
Assignee: general → crowder
Status: NEW → ASSIGNED
Attachment #267471 - Flags: review?(mrbkap)
Comment on attachment 267471 [details] [diff] [review]
adds "(void 0)" atom and uses it for toString of JSVAL_VOID

Is it really worth it to add an atom for this? I feel like a static const char[] in jsstr.c would suffice here. Tell me if I'm wrong, though!
(Reporter)

Comment 5

11 years ago
I think toString should return "undefined" for web compatibility and avoiding surprise, but toSource/uneval should return "(void 0)" for correctness.
(Assignee)

Comment 6

11 years ago
Created attachment 267477 [details] [diff] [review]
addressing Jesse's suggestion

Still using an atom here, for now, but I can be convinced otherwise.
Attachment #267471 - Attachment is obsolete: true
Attachment #267477 - Flags: review?(mrbkap)
Attachment #267471 - Flags: review?(mrbkap)
Attachment #267477 - Flags: review?(mrbkap) → review+
I can see how using a C string constant and inflating it into a new JS string every time would be both (a) more code than this patch, and (b) costly in time and (if done often) space. Sometimes the smallest patch is the best, yay. Cc'ing Igor to get his thoughts.

/be
(Assignee)

Updated

11 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED

Comment 8

11 years ago
/cvsroot/mozilla/js/tests/js1_5/extensions/regress-375801.js,v  <--  regress-375801.js
initial revision: 1.1
Flags: in-testsuite+

Comment 9

11 years ago
verified fixed 1.9.0 linux/mac*/windows.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.