The default bug view has changed. See this FAQ.

Emit JSOP_OBJECT when array initializer contains negative numbers

RESOLVED FIXED in mozilla15

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: jandem, Assigned: evilpie)

Tracking

unspecified
mozilla15
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

6 years ago
Noticed this when reducing some testcase:
--
js> [1]; dis()  
main:
00000:  object [1]
00003:  popv
--
And this:
--
js> [-1]; dis()    
main:
00000:  newarray 1
00004:  zero
00005:  int8 -1
00007:  initelem
00008:  endinit
--
The problem seems to be that JSParseNode::getConstantValue does not support negative numbers.

I don't know if this will win performance somewhere. SS base64 uses an array with negative numbers though and some benchmarks like to use arrays as vectors like this: [-1, 0, 1].
(Reporter)

Comment 1

6 years ago
Created attachment 524651 [details] [diff] [review]
Patch

Brian asking your r? because you wrote JSParseNode::isConstant. The problem was that -10 is constant folded to one token after parsing the initializer.
Attachment #524651 - Flags: review?(bhackett1024)
This will I think break in JSParseNode::getConstantValue if it is not also updated.  Can you add a testcase or update tests/basic/testInitSingleton?
(Reporter)

Updated

6 years ago
Attachment #524651 - Flags: review?(bhackett1024)
(Assignee)

Updated

5 years ago
Assignee: jdemooij → evilpies
(Assignee)

Comment 3

5 years ago
Created attachment 584467 [details] [diff] [review]
do FoldConstants for every node

Brain is this a sensible approach to do this, or is there some reason that we can't doing folding there?
Attachment #584467 - Flags: feedback?(bhackett1024)
Comment on attachment 584467 [details] [diff] [review]
do FoldConstants for every node

I think FoldConstants is fine to use here, and should be more robust.
Attachment #584467 - Flags: feedback?(bhackett1024) → review+
(Assignee)

Comment 5

5 years ago
Created attachment 618364 [details] [diff] [review]
Call FoldConstants of every node.

Finally came around finishing this. I know the tests are kinda dense, but I hope we test constant folding in enough other places.
Attachment #524651 - Attachment is obsolete: true
Attachment #584467 - Attachment is obsolete: true
Attachment #618364 - Flags: review?(bhackett1024)
Attachment #618364 - Flags: review?(bhackett1024) → review+
(Assignee)

Comment 6

5 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/865c938824b1
https://hg.mozilla.org/mozilla-central/rev/865c938824b1
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla15
You need to log in before you can comment on or make changes to this bug.