Last Comment Bug 749786 - prototype int64/uint64 value objects, with operators and literal syntax
: prototype int64/uint64 value objects, with operators and literal syntax
Status: ASSIGNED
: dev-doc-needed
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal with 8 votes (vote)
: ---
Assigned To: Brendan Eich [:brendan]
:
Mentors:
http://wiki.ecmascript.org/doku.php?i...
Depends on:
Blocks: 752441 757256 JSIL es7 898664
  Show dependency treegraph
 
Reported: 2012-04-27 13:59 PDT by Brendan Eich [:brendan]
Modified: 2015-06-21 07:36 PDT (History)
38 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
work in progress, does + on {int64,number} (62.83 KB, patch)
2012-05-03 19:02 PDT, Brendan Eich [:brendan]
no flags Details | Diff | Splinter Review
work in progress, more binary ops done in interpreter (bitwise, ===/!==, unary still todo) (90.20 KB, patch)
2012-05-04 17:20 PDT, Brendan Eich [:brendan]
no flags Details | Diff | Splinter Review
WIP: bitwise and shift ops done, fixed silly snprintf buf size bugs (98.52 KB, patch)
2012-05-05 12:19 PDT, Brendan Eich [:brendan]
no flags Details | Diff | Splinter Review
WIP, all operators implemented, also 1234L suffix a la C# (159.71 KB, patch)
2012-05-07 00:48 PDT, Brendan Eich [:brendan]
no flags Details | Diff | Splinter Review
WIP, all operators implemented, also 1234L suffix a la C# (159.78 KB, patch)
2012-05-07 00:50 PDT, Brendan Eich [:brendan]
no flags Details | Diff | Splinter Review
WIP, all operators implemented, also 1234L suffix a la C# (also hex literals) (161.10 KB, patch)
2012-05-07 01:03 PDT, Brendan Eich [:brendan]
luke: feedback+
Details | Diff | Splinter Review
WIP, add UL suffix, LxUL and ULxL binary operators (173.74 KB, patch)
2012-06-25 06:08 PDT, Brendan Eich [:brendan]
no flags Details | Diff | Splinter Review
WIP, add UL suffix, LxUL and ULxL binary operators (171.62 KB, patch)
2012-07-25 16:09 PDT, Brendan Eich [:brendan]
no flags Details | Diff | Splinter Review
rebased (173.06 KB, patch)
2012-07-31 15:17 PDT, Brendan Eich [:brendan]
no flags Details | Diff | Splinter Review
rebased yet again (175.78 KB, patch)
2012-08-20 18:44 PDT, Brendan Eich [:brendan]
no flags Details | Diff | Splinter Review
rebased, sad to see nice JSObject methods become static (174.55 KB, patch)
2012-08-24 10:45 PDT, Brendan Eich [:brendan]
no flags Details | Diff | Splinter Review
rebased, sorry for the delay (177.18 KB, patch)
2012-10-17 15:17 PDT, Brendan Eich [:brendan]
no flags Details | Diff | Splinter Review
rebased, starting to look at the JITs (176.87 KB, patch)
2012-10-30 18:00 PDT, Brendan Eich [:brendan]
no flags Details | Diff | Splinter Review
rebased, looking at JITs on upcoming trip (only time I have to hack :-/) (177.08 KB, patch)
2012-11-09 15:55 PST, Brendan Eich [:brendan]
no flags Details | Diff | Splinter Review
rebased (183.31 KB, patch)
2012-11-24 22:58 PST, Brendan Eich [:brendan]
no flags Details | Diff | Splinter Review
rebased (182.86 KB, patch)
2012-12-07 14:34 PST, Brendan Eich [:brendan]
no flags Details | Diff | Splinter Review
rebased for 2013 (190.69 KB, patch)
2013-01-02 10:17 PST, Brendan Eich [:brendan]
no flags Details | Diff | Splinter Review
rebased (180.35 KB, patch)
2013-01-19 18:53 PST, Brendan Eich [:brendan]
no flags Details | Diff | Splinter Review
heckuva rebase (180.31 KB, patch)
2013-01-26 21:24 PST, Brendan Eich [:brendan]
no flags Details | Diff | Splinter Review
rebased yet again (180.22 KB, patch)
2013-01-29 08:21 PST, Brendan Eich [:brendan]
no flags Details | Diff | Splinter Review
big-ass rebase (178.07 KB, patch)
2013-03-14 11:38 PDT, Brendan Eich [:brendan]
no flags Details | Diff | Splinter Review
copy/paste fix to last rev (178.07 KB, patch)
2013-03-14 21:33 PDT, Brendan Eich [:brendan]
no flags Details | Diff | Splinter Review
rebased past arrow functions (yay!) (178.01 KB, patch)
2013-03-19 13:07 PDT, Brendan Eich [:brendan]
no flags Details | Diff | Splinter Review
rebase (178.13 KB, patch)
2013-03-28 02:23 PDT, Brendan Eich [:brendan]
no flags Details | Diff | Splinter Review
rebase-o-rama (178.15 KB, patch)
2013-04-05 11:29 PDT, Brendan Eich [:brendan]
no flags Details | Diff | Splinter Review
rebased (173.32 KB, patch)
2013-04-29 22:38 PDT, Brendan Eich [:brendan]
no flags Details | Diff | Splinter Review
rebased (171.90 KB, patch)
2013-05-04 16:06 PDT, Brendan Eich [:brendan]
no flags Details | Diff | Splinter Review
rebased (171.87 KB, patch)
2013-05-09 16:29 PDT, Brendan Eich [:brendan]
no flags Details | Diff | Splinter Review
gigundo-rebase (177.03 KB, patch)
2013-05-21 07:00 PDT, Brendan Eich [:brendan]
no flags Details | Diff | Splinter Review
not-so-gigundo-rebase (177.08 KB, patch)
2013-05-29 15:07 PDT, Brendan Eich [:brendan]
no flags Details | Diff | Splinter Review
stupendo-rebase (173.58 KB, patch)
2013-06-22 14:23 PDT, Brendan Eich [:brendan]
no flags Details | Diff | Splinter Review
mini-rebase on top of stupendo-rebase (173.56 KB, patch)
2013-06-23 19:12 PDT, Brendan Eich [:brendan]
no flags Details | Diff | Splinter Review
moderate rebase (174.28 KB, patch)
2013-07-07 14:30 PDT, Brendan Eich [:brendan]
no flags Details | Diff | Splinter Review
yet another rebase (174.04 KB, patch)
2013-07-08 14:27 PDT, Brendan Eich [:brendan]
no flags Details | Diff | Splinter Review

Description Brendan Eich [:brendan] 2012-04-27 13:59:57 PDT
Patch coming along, hope to post it later today.

/be
Comment 1 Brendan Eich [:brendan] 2012-04-27 14:00:53 PDT
Doing only int64 and uint64 to start.

/be
Comment 2 Dave Herman [:dherman] 2012-04-28 09:43:20 PDT
Brendan: thanks for prototyping this. Can you include these two features in your implementation?

- a predicate to test whether an object is a value object: Object.isValue
- a check on WeakMap to disallow the use of value objects as keys

Thanks,
Dave
Comment 3 Brendan Eich [:brendan] 2012-04-28 10:45:14 PDT
(In reply to Dave Herman [:dherman] from comment #2)
> - a predicate to test whether an object is a value object: Object.isValue

Will do, that's in the strawman already.

> - a check on WeakMap to disallow the use of value objects as keys

Could you please update the strawman to talk about this, including rationale? Thanks,

/be
Comment 4 Dave Herman [:dherman] 2012-04-28 11:33:26 PDT
> > - a predicate to test whether an object is a value object: Object.isValue
> 
> Will do, that's in the strawman already.

OK. You asked on IRC what Object.isValue should return for non-object values. My first reaction was throw, since it's meant to be for objects. But I can't come up with another name that's not completely stilted:

    isSynthetic
    isDeep
    isStructural
    isExtensional
    isForgeable
    isSynthetic

Meh. Probably the most straightforward thing is for Object.isValue to return true for all non-object values and all non-reference objects, and false for all reference objects.

Even though I loathe the terminology of "value object." What can you do, I wrote the spec. ;-P

> > - a check on WeakMap to disallow the use of value objects as keys
> 
> Could you please update the strawman to talk about this, including
> rationale? Thanks,

Added.

Dave
Comment 5 Dave Herman [:dherman] 2012-04-28 18:45:28 PDT
What are you doing about the coercion rules for the various operators? Just kinda winging it, or leaving it out completely for the first cut? (I haven't had a chance to go through the spec and work out the details yet.)

Dave
Comment 6 Brendan Eich [:brendan] 2012-05-03 19:02:00 PDT
Created attachment 620929 [details] [diff] [review]
work in progress, does + on {int64,number}

More soon, sorry for the delay. See the comment in jsclass.h for the operator dispatch design.

/be
Comment 7 Brendan Eich [:brendan] 2012-05-03 19:09:25 PDT
> the most straightforward thing is for Object.isValue to return true for all non-object
> values and all non-reference objects, and false for all reference objects.

This wins, indeed.

/be
Comment 8 Brendan Eich [:brendan] 2012-05-04 17:17:59 PDT
(In reply to Dave Herman [:dherman] from comment #5)
> What are you doing about the coercion rules for the various operators?

Strictness reigns, some examples:

js> i = int64(1.5)
typein:1: TypeError: can't convert 1.5 to int64

js> i = int64(9223372036854775808)
typein:2: TypeError: can't convert 9223372036854776000 to int64

js> i = int64('9223372036854775808')
typein:3: TypeError: can't convert "9223372036854775808" to int64

js> i = int64('9223372036854775807') 
int64('9223372036854775807'

js> j = int64('-9223372036854775808')
int64(-9223372036854775808)

js> j = int64('-9223372036854775809') 
typein:6: TypeError: can't convert "-9223372036854775809" to int64

js> u = uint64(-1)
typein:7: TypeError: can't convert -1 to uint64

js> u = uint64(18446744073709551616)
typein:8: TypeError: can't convert 18446744073709552000 to uint64

js> u = uint64('18446744073709551616')
typein:9: TypeError: can't convert "18446744073709551616" to uint64

js> u = uint64('18446744073709551615') 
uint64('18446744073709551615')

Working on bitwise binary ops, then on to unaries.

Per the comment in jsclass.h based in Christian Hansen's 2009 proposal, I'm also not (yet) supporting implicit toString via +:

js> i = int64(42)
int64(42)
js> i+9
int64(51)
js> i+'hi'
typein:3: TypeError: no binary operator function found for + use
js> i.toString()
"42"
js> i.toString()+'hi'
"42hi"

Spec comment:

 * When executing the '+' operator in 'A + B' where A and B refer to value
 * objects, do the following:
 *
 *  1. Get the value of property LOP_PLUS in A, call the result P
 *  2. If P is not a list throw a TypeError: no '+' operator
 *  3. Get the property ROP_PLUS in B, call the result Q
 *  4. If Q is not a list give a TypeError: no '+' operator
 *  5. Intersect the lists P and Q, call the result R
 *  6. If R is empty throw a TypeError: no '+' operator
 *  7. If R has more than one element throw a TypeError: ambiguity
 *  8. If R[0], call it F, is not a function throw a TypeError
 *  9. Evaluate F(A, B) and return the result

Two options:

A. Change steps 2 and 4 to fall back on old + behavior rather than throw a TypeError.

B. Add operator functions to String.prototype as well as Number.prototype (and presumably to Boolean.prototype too).

Comments welcome. I'll put a rebased patch up for backup and anyone who wants to play with it.

/be

/be
Comment 9 Brendan Eich [:brendan] 2012-05-04 17:20:27 PDT
Created attachment 621225 [details] [diff] [review]
work in progress, more binary ops done in interpreter (bitwise, ===/!==, unary still todo)
Comment 10 Brendan Eich [:brendan] 2012-05-05 12:19:41 PDT
Created attachment 621333 [details] [diff] [review]
WIP: bitwise and shift ops done, fixed silly snprintf buf size bugs

Better:

js> u = uint64("0xFFFFFFFFFFFFFFFF")
uint64('18446744073709551615')
js> u >>> 1
uint64('9223372036854775807')
js> u >> 1
uint64('9223372036854775807')
js> i = int64("0xFFFFFFFFFFFFFFFF")  
typein:5: TypeError: can't convert "0xFFFFFFFFFFFFFFFF" to int64
js> i = int64("0x7FFFFFFFFFFFFFFF") 
int64('9223372036854775807')
js> i <<= 1
int64(-2)
js> i >>> 1
int64('9223372036854775807')
js> i >> 1
int64(-1)

/be
Comment 11 Brendan Eich [:brendan] 2012-05-07 00:48:51 PDT
Created attachment 621526 [details] [diff] [review]
WIP, all operators implemented, also 1234L suffix a la C#

js> 1234L
1234L
js> i = 9223372036854775807L  
9223372036854775807L
js> i.toString(16)
"7fffffffffffffff"
js> u = 18446744073709551615L
18446744073709551615L
js> v = ~u
0L
js> u == v
false
js> u === v
false
js> w = ~v
18446744073709551615L
js> u == w
true
js> u === w
true
js> if (v) print('nope')
js> if (u) print('yep')
yep

This shows ~, ==, === working as expected. To preserve the identities listed in jsclass.h, ! and != cannot be overloaded, only ==; ditto > and >=, only < and <=.

Yes, objects are not always truthy -- value objects can be falsy. This required undoing the changes from bug 409476, but in doing so I removed some useless proxy veneer on ToBoolean, and studied its workload a bit (see the comments and metering code, to be turned off before pushing the final patch here).

Moar and uglier macros, oh how I wish templates could take operators as static parameters. Help me, Obi-Wan!

Still, pretty sweet to have int64/uint64 prototyped in SpiderMonkey. Onward in a separate bug to TI and JITting for unboxed high performance!

/be
Comment 12 Brendan Eich [:brendan] 2012-05-07 00:50:20 PDT
Created attachment 621527 [details] [diff] [review]
WIP, all operators implemented, also 1234L suffix a la C#

Fixed so int64.prototype and uint64.prototype are big enough to hold useless 0 64-bit values.

/be
Comment 13 Brendan Eich [:brendan] 2012-05-07 01:03:59 PDT
Created attachment 621528 [details] [diff] [review]
WIP, all operators implemented, also 1234L suffix a la C# (also hex literals)

Hex only, no octal. The only good reason for octal these days (that I know of, and I'm an old octal fan from my DEC days) is Unix file modes -- but those are never gonna be 64 bits :-P.

/be
Comment 14 Brendan Eich [:brendan] 2012-05-07 08:46:37 PDT
Still TODO in this bug's patch:

* tests...
* API (JS_New{I,Ui}nt64Object, value getters)
* redo ctypes/Ctypes.cpp to use builtin/IntUtility.h
* jsreflect.cpp needs updating
* frontend/FoldConstants.cpp ditto
* better js::StrictlyEquals solution than memcmp

/be
Comment 15 Brendan Eich [:brendan] 2012-05-07 08:51:57 PDT
More TODO notes:

* fix ambiguous operator error message/args (no useless native function decompilation)
* verify ToBoolean stats with jits off

/be
Comment 16 Luke Wagner [:luke] 2012-05-07 09:54:16 PDT
Comment on attachment 621528 [details] [diff] [review]
WIP, all operators implemented, also 1234L suffix a la C# (also hex literals)

Review of attachment 621528 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Brendan Eich [:brendan] from comment #11)
> Moar and uglier macros, oh how I wish templates could take operators as
> static parameters. Help me, Obi-Wan!

The canonical way would be to pass a function object.  Here we could actually use the C++ stdlib: #include <functional> contains std::{plus, minus, multiplies, bit_and, etc}.

In general, do you think we could kill *all* the big macros added by this patch (not just the one that takes an op) with inline functions?

Random nit: for symmetry with JSObject::{isNumber,isBoolean,isString}, could it be JSObject::isValue?

::: js/src/jsinterp.cpp
@@ +857,5 @@
> +
> +                // FIXME: not deep, no -0/0 equiv, no NaN handling. good enough for int64/uint64.
> +                size_t size = lobj.sizeOfThis();
> +                JS_ASSERT(size == robj.sizeOfThis());
> +                *equal = memcmp(&lobj + 1, &robj + 1, size - sizeof(JSObject)) == 0;

I know this is temporary, but I was wondering if, in the final product, there could be a class ValueObject : public JSObject (where class Int64Object : public ValueObject) and then we could put the value-polymorphic methods (like, for this case, 'equal') on ValueObject.
Comment 17 Brendan Eich [:brendan] 2012-05-07 11:57:16 PDT
More TODO:

* int64 x uint64 binary operators
* http://www.cplusplus.com/reference/std/functional/plus/ etc., per Luke's comment 16

/be
Comment 18 Terrence Cole [:terrence] 2012-05-21 16:07:21 PDT
*** Bug 757256 has been marked as a duplicate of this bug. ***
Comment 19 Brendan Eich [:brendan] 2012-06-25 06:08:08 PDT
Created attachment 636289 [details] [diff] [review]
WIP, add UL suffix, LxUL and ULxL binary operators
Comment 20 Brendan Eich [:brendan] 2012-07-25 16:09:39 PDT
Created attachment 645920 [details] [diff] [review]
WIP, add UL suffix, LxUL and ULxL binary operators

Rebased (painfully).

/be
Comment 21 Brendan Eich [:brendan] 2012-07-31 15:17:01 PDT
Created attachment 647708 [details] [diff] [review]
rebased
Comment 22 Brendan Eich [:brendan] 2012-08-20 18:44:49 PDT
Created attachment 653618 [details] [diff] [review]
rebased yet again

Thanks to Luke for pointers re: Handle<t>::fromMarkedLocation.

Second patch in mq that de-macroizes aggressively (but not completely -- macros have their uses) coming within the week.

/be
Comment 23 Brendan Eich [:brendan] 2012-08-24 10:45:13 PDT
Created attachment 655055 [details] [diff] [review]
rebased, sad to see nice JSObject methods become static
Comment 24 nemo 2012-09-04 08:24:01 PDT
Nice.  Looking forward to this, as it will eliminate the int64 hacks in emscripten.
Our cross-compile uses 64 bit ints extensively, so we take a huge hit right now, even with the double trick and specialcasing to avoid overflow.
Comment 25 joran 2012-10-09 23:30:26 PDT
Thanks, also looking forward. Anyone on WebKit working on this?
Comment 26 Brendan Eich [:brendan] 2012-10-17 15:17:14 PDT
Created attachment 672544 [details] [diff] [review]
rebased, sorry for the delay
Comment 27 Brendan Eich [:brendan] 2012-10-30 18:00:30 PDT
Created attachment 676871 [details] [diff] [review]
rebased, starting to look at the JITs
Comment 28 Brendan Eich [:brendan] 2012-11-09 15:55:16 PST
Created attachment 680269 [details] [diff] [review]
rebased, looking at JITs on upcoming trip (only time I have to hack :-/)
Comment 29 Brendan Eich [:brendan] 2012-11-24 22:58:35 PST
Created attachment 684917 [details] [diff] [review]
rebased
Comment 30 Brendan Eich [:brendan] 2012-12-07 14:34:30 PST
Created attachment 689939 [details] [diff] [review]
rebased
Comment 31 Brendan Eich [:brendan] 2013-01-02 10:17:26 PST
Created attachment 697075 [details] [diff] [review]
rebased for 2013

I will try to unify JSCLASS_EMULATES_UNDEFINED (JSC's "masquerades_as" is longer but a better trope given the deception :-P) with JSCLASS_VALUE_OBJECT.

/be
Comment 32 Brendan Eich [:brendan] 2013-01-19 18:53:59 PST
Created attachment 704290 [details] [diff] [review]
rebased
Comment 33 Brendan Eich [:brendan] 2013-01-26 21:24:10 PST
Created attachment 706847 [details] [diff] [review]
heckuva rebase
Comment 34 Brendan Eich [:brendan] 2013-01-29 08:21:40 PST
Created attachment 707648 [details] [diff] [review]
rebased yet again
Comment 35 Brendan Eich [:brendan] 2013-03-14 11:38:56 PDT
Created attachment 725038 [details] [diff] [review]
big-ass rebase
Comment 36 Brendan Eich [:brendan] 2013-03-14 21:33:30 PDT
Created attachment 725254 [details] [diff] [review]
copy/paste fix to last rev
Comment 37 Brendan Eich [:brendan] 2013-03-19 13:07:44 PDT
Created attachment 726861 [details] [diff] [review]
rebased past arrow functions (yay!)
Comment 38 Jim Blandy :jimb 2013-03-19 13:21:57 PDT
Comment on attachment 726861 [details] [diff] [review]
rebased past arrow functions (yay!)

Review of attachment 726861 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/builtin/Object.cpp
@@ +969,5 @@
> +static JSBool
> +obj_isObject(JSContext *cx, unsigned argc, Value *vp)
> +{
> +    RootedValue v(cx);
> +    if (!GetFirstArgument(cx, argc, vp, "Object.isValue", &v))

"Object.isObject" in this one, right?
Comment 39 Brendan Eich [:brendan] 2013-03-28 02:23:04 PDT
Created attachment 730591 [details] [diff] [review]
rebase
Comment 40 Brendan Eich [:brendan] 2013-04-05 11:29:13 PDT
Created attachment 733958 [details] [diff] [review]
rebase-o-rama
Comment 41 Brendan Eich [:brendan] 2013-04-29 22:38:44 PDT
Created attachment 743456 [details] [diff] [review]
rebased
Comment 42 Brendan Eich [:brendan] 2013-05-04 16:06:38 PDT
Created attachment 745589 [details] [diff] [review]
rebased
Comment 43 Brendan Eich [:brendan] 2013-05-09 16:29:41 PDT
Created attachment 747672 [details] [diff] [review]
rebased
Comment 44 Brendan Eich [:brendan] 2013-05-21 07:00:00 PDT
Created attachment 752180 [details] [diff] [review]
gigundo-rebase
Comment 45 Brendan Eich [:brendan] 2013-05-29 15:07:51 PDT
Created attachment 755630 [details] [diff] [review]
not-so-gigundo-rebase

Hope to have some travel downtime this week to do tests and cleanup work...

/be
Comment 46 Brendan Eich [:brendan] 2013-06-22 14:23:33 PDT
Created attachment 766358 [details] [diff] [review]
stupendo-rebase
Comment 47 Brendan Eich [:brendan] 2013-06-23 19:12:47 PDT
Created attachment 766507 [details] [diff] [review]
mini-rebase on top of stupendo-rebase

Just jsbool.cpp, applied with patch --fuzz=4 but here it is for you loyal testers (you are there, aren't you?).

/be
Comment 48 Till Schneidereit [:till] 2013-06-24 11:18:26 PDT
> stupendo-rebase

I'm curious: what is the reason for not getting this reviewed and into the tree, disabling it in everything but Nightly?
Comment 49 David Bruant 2013-06-25 00:31:30 PDT
(In reply to Till Schneidereit [:till] from comment #48)
> > stupendo-rebase
> 
> I'm curious: what is the reason for not getting this reviewed and into the
> tree, disabling it in everything but Nightly?
I love this idea! I know that, personally, I would be more likely to play with it if it lands on Nightly. It's probably the case for most JS devs.
I would write the doc and devs could be encouraged to check it out even more if they have doc.
Comment 50 Till Schneidereit [:till] 2013-06-25 04:21:34 PDT
So, there's this discussion about landing new web-facing features going on over on dev-platform:
https://groups.google.com/forum/#!topic/mozilla.dev.platform/ihjMCbOM3qA

I don't know enough about the state of discussion about value objects to know that landing them would be in accordance with the policy that's converged on. It seems to me that there *is* a lot of interest in value objects, and would love to get them into the hands of developers in a restricted fashion, at least.

Also, those rebases must be painful.
Comment 51 Brandon Benvie [:benvie] 2013-06-26 16:56:48 PDT
(In reply to Till Schneidereit [:till] from comment #50)
> So, there's this discussion about landing new web-facing features going on
> over on dev-platform:
> https://groups.google.com/forum/#!topic/mozilla.dev.platform/ihjMCbOM3qA
> 
> I don't know enough about the state of discussion about value objects to
> know that landing them would be in accordance with the policy that's
> converged on. It seems to me that there *is* a lot of interest in value
> objects, and would love to get them into the hands of developers in a
> restricted fashion, at least.
> 
> Also, those rebases must be painful.

I'm fairly sure the idea is that if something is prefed off then it's fair game to have it in nightly/beta.

I also think it would be excellent if this was landed (and hidden behind a pref).
Comment 52 Brendan Eich [:brendan] 2013-07-07 14:30:57 PDT
Created attachment 771877 [details] [diff] [review]
moderate rebase

I want to get into landing shape this week...

/be
Comment 53 AWAY Tom Schuster [:evilpie] 2013-07-08 08:44:43 PDT
Comment on attachment 771877 [details] [diff] [review]
moderate rebase

Review of attachment 771877 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry, this probably seems quite passive aggressive, but I started looking at this on the weekend and sadly never good into looking at the real deal. So this is just some style nits :(

::: js/public/Value.h
@@ -911,5 @@
>          data = MAGIC_TO_JSVAL_IMPL(why);
>      }
>  
>      bool setNumber(uint32_t ui) {
> -        if (ui > JSVAL_INT_MAX) {

Seems unrelated.

::: js/src/builtin/Array.js
@@ +22,5 @@
>          return -1;
>  
>      var k;
>      /* Step 7. */
> +    if (n >= 0) {

This seems unrelated.

::: js/src/builtin/Int64Object.cpp
@@ +1,2 @@
> +/* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*-
> + * vim: set ts=8 sw=4 et tw=99 ft=cpp:

Please use the short MPL2.0, applies to other files as well.

::: js/src/builtin/IntUtility.h
@@ +1,1 @@
> +/* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*-

Interesting Waldo should have a look at this, we try to have most of our conversion utilities in mfbt/

::: js/src/builtin/Object.cpp
@@ +962,5 @@
> +    RootedValue v(cx);
> +    if (!GetFirstArgument(cx, args, "Object.isValue", &v))
> +        return false;
> +
> +    vp->setBoolean(!v.isObject() || v.toObject().isValue());

This should be: args.rval().setBoolean()

@@ +971,5 @@
> +obj_isObject(JSContext *cx, unsigned argc, Value *vp)
> +{
> +    CallArgs args = CallArgsFromVp(argc, vp);
> +    RootedValue v(cx);
> +    if (!GetFirstArgument(cx, args, "Object.isValue", &v))

This seems unrelated as well. Also note Object.isObject.

@@ +974,5 @@
> +    RootedValue v(cx);
> +    if (!GetFirstArgument(cx, args, "Object.isValue", &v))
> +        return false;
> +
> +    vp->setBoolean(v.isObject());

args.rval().setBoolean()

::: js/src/builtin/TestingFunctions.cpp
@@ +511,5 @@
>      }
>  
> +    JSBool b;
> +    if (!ToBoolean(cx, vp[2], &b))
> +        return JS_FALSE;

false

::: js/src/jsapi.cpp
@@ +281,5 @@
>            case 'b':
> +          {
> +            bool b;
> +            if (!ToBoolean(cx, *sp, &b))
> +                return JS_FALSE;

false. I am starting to think that fallible ToBoolean could be in an other patch.

::: js/src/jsapi.h
@@ +1171,5 @@
>  
>  /*
>   * A jsid is an identifier for a property or method of an object which is
> + * either an interned string, a 31-bit signed integer, an object (a private
> + * name object, or E4X QName object if XML is enabled), or an internal code

whooa.

::: js/src/jsbool.cpp
@@ +207,5 @@
>      JS_ASSERT(obj);
>      return obj->as<BooleanObject>().unbox();
>  }
> +
> +#define METERING 1

For landing we probably want to remove this.
Comment 54 Brendan Eich [:brendan] 2013-07-08 14:27:42 PDT
Created attachment 772324 [details] [diff] [review]
yet another rebase
Comment 55 nemo 2013-09-09 07:20:15 PDT
Apologies for a non-constructive comment, but I was wondering.  Is this going to end up in nightlies as discussed in comment #49 ?
I'm wondering because:
https://wiki.mozilla.org/Javascript:SpiderMonkey:OdinMonkey 
no longer lists this bug as of:
https://wiki.mozilla.org/index.php?title=Javascript%3ASpiderMonkey%3AOdinMonkey&diff=677040&oldid=669697

Did merging this turn out to be too much trouble/risk?
Comment 56 Luke Wagner [:luke] 2013-09-09 07:59:11 PDT
For a while the official plan to get float32 codegen in asm.js was to leverage the float32 value object.  That is still the long-term plan, but we found a more immediate solution with Math.fround (bug 900120).
Comment 57 Niko Matsakis [:nmatsakis] 2013-11-13 13:50:40 PST
FYI: As part of the work on SIMD (bug 904913), we'll probably wind up reimplementing/porting a good deal of this work in terms of typed objects (bug 578700).
Comment 58 Niko Matsakis [:nmatsakis] 2013-11-13 16:37:04 PST
Having read the patch, I see there's a lot in here! I think the only parts we'll need for the SIMD work is to implement the core idea of a JSObject* that acts like a value (presumably it'll be a TypedObject* that overrides === and typeof). I imagine we'll be cribbing quite a bit from this patch with respect to how to go about making === and typeof more overridable than they are today. The other parts of the patch -- operator overloading, parsing uint64s, etc -- should probably stay in this bug for the time being, though I guess we'll eventually want to overload the operators for SIMD as well to make things feel as natural as possible.

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