Closed Bug 373843 Opened 13 years ago Closed 8 years ago

dis() seems to over-escape strings

Categories

(Core :: JavaScript Engine, defect, minor)

x86
macOS
defect
Not set
minor

Tracking

()

RESOLVED WORKSFORME

People

(Reporter: jruderman, Assigned: h4writer)

Details

(Keywords: testcase)

Attachments

(1 file, 1 obsolete file)

js> dis(function() { return "c\\d"; })
flags: LAMBDA INTERPRETED
main:
00000:  string "c\\\\d"
00003:  return
00004:  stop
I have no idea when it got fixed, nor what the related bug report is.
But executing that piece of code on mozilla-inbound gives me the following

flags: LAMBDA NULL_CLOSURE
loc     op
-----   --
main:
00000:  string "c\\d"
00003:  return
00004:  stop

Source notes:
 ofs  line    pc  delta desc     args
---- ---- ----- ------ -------- ------

So I assume the bug got fixed in the meantime.
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → WORKSFORME
Flags: in-testsuite?
@Ryan: I'm not sure how I can make a testcase out of this?
If one can't be made, the in-testsuite? flag can be set to (-).
You can make a testcase by using disassemble(), which returns a string, instead of dis().

These are roughly equivalent:

              dis(function() { return "c\\d"; })
print(disassemble(function() { return "c\\d"; }))
Attached patch Testcase (obsolete) — Splinter Review
Ty, I've always used "dis" instead of "disassemble".
I included a testcase.
Assignee: general → hv1989
Status: RESOLVED → REOPENED
Attachment #577961 - Flags: review?(jruderman)
Resolution: WORKSFORME → ---
Comment on attachment 577961 [details] [diff] [review]
Testcase

I think disassemble is only available in debug builds, so please mark the test as debug-only.

Any particular reason to use assertEq rather than reportCompare for this test?
Attachment #577961 - Flags: review?(jruderman) → review+
There was not a particular reason why I didn't use "reportCompare". I just looked to the other testcases and try to mimic them. I noticed in the newer testcases the "reportCompare" is almost never used.

I also tried to add debugMode in jstests.list, but when I tried with a non debug build "./jstests [JS] js1_8_5/regress" it tripped on the disassembler not being available. So that's why I changed the testcase a bit and only run the test if the function disassemble is available.
Attachment #577961 - Attachment is obsolete: true
Attachment #577982 - Flags: review?(jruderman)
Comment on attachment 577982 [details] [diff] [review]
Testcase (take 2)

Looks good to me.
Attachment #577982 - Flags: review?(jruderman) → review+
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/5058cbe2e897
Status: REOPENED → RESOLVED
Closed: 8 years ago8 years ago
Flags: in-testsuite? → in-testsuite+
Keywords: checkin-needed
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.