Last Comment Bug 648526 - Emit JSOP_OBJECT when array initializer contains negative numbers
: Emit JSOP_OBJECT when array initializer contains negative numbers
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla15
Assigned To: Tom Schuster [:evilpie]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-04-08 07:18 PDT by Jan de Mooij [:jandem]
Modified: 2012-05-04 13:39 PDT (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (919 bytes, patch)
2011-04-08 10:11 PDT, Jan de Mooij [:jandem]
no flags Details | Diff | Splinter Review
do FoldConstants for every node (2.33 KB, patch)
2011-12-27 12:37 PST, Tom Schuster [:evilpie]
bhackett1024: review+
Details | Diff | Splinter Review
Call FoldConstants of every node. (3.19 KB, patch)
2012-04-25 11:14 PDT, Tom Schuster [:evilpie]
bhackett1024: review+
Details | Diff | Splinter Review

Description Jan de Mooij [:jandem] 2011-04-08 07:18:12 PDT
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].
Comment 1 Jan de Mooij [:jandem] 2011-04-08 10:11:25 PDT
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.
Comment 2 Brian Hackett (:bhackett) 2011-04-08 12:02:44 PDT
This will I think break in JSParseNode::getConstantValue if it is not also updated.  Can you add a testcase or update tests/basic/testInitSingleton?
Comment 3 Tom Schuster [:evilpie] 2011-12-27 12:37:09 PST
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?
Comment 4 Brian Hackett (:bhackett) 2011-12-28 06:19:37 PST
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.
Comment 5 Tom Schuster [:evilpie] 2012-04-25 11:14:54 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.