704071 - JSON output support for float and float4

Status: VERIFIED FIXED

(Tamarin Graveyard :: Virtual Machine, defect, P3)

Description

[Lars T Hansen]

JSON being JSON, float has to be formatted as a Number, and a float4 as an Array of Number.

Comment 1

[Lars T Hansen]

Attachment: Some preliminary work

Comment 2

[Lars T Hansen]

Attachment: Alternative solution

Formats a float4 as a property list (object) instead.  For reasons I don't understand yet this patch does not work properly when I call it thusly:

JSON.stringify(float4(1,2,3,4), null, "   ")

as it then runs the property values together on a line like this:

{4321
}

but what I had expected is this (for some property order):

{
   "x": 1,
   "y": 2,
   "z": 3,
   "w": 4
}

To be continued later - not high priority.

Comment 3

[Felix S. Klock II [:pnkfelix, :fklock]]

For the first release, I'd just make the toJSON serialization pure AS code in JSON.as, rather than mucking about in the JSONClass.cpp logic.

Comment 4

[Lars T Hansen]

(In reply to Felix S Klock II from comment #3)
> For the first release, I'd just make the toJSON serialization pure AS code
> in JSON.as, rather than mucking about in the JSONClass.cpp logic.

Aha.  Where would you put that in?

(Also remember we need to ifdef the code, even in AS3 - certainly we can do that with our configuration system, we're already doing it for the float class, but it still needs to be accounted for.)

Comment 5

[Felix S. Klock II [:pnkfelix, :fklock]]

(In reply to Lars T Hansen from comment #4)
> (In reply to Felix S Klock II from comment #3)
> > For the first release, I'd just make the toJSON serialization pure AS code
> > in JSON.as, rather than mucking about in the JSONClass.cpp logic.
> 
> Aha.  Where would you put that in?
> 
> (Also remember we need to ifdef the code, even in AS3 - certainly we can do
> that with our configuration system, we're already doing it for the float
> class, but it still needs to be accounted for.)

See for example the (purposefully trivial) toJSON methods in core/{Date,XML,ByteArray}.as

The Float4 one would be slightly more complicated, but not too much so; just assemble the array or object, and return it.

Comment 6

[Lars T Hansen]

I will look into that.

My concern is that a float4 value in the VM is not a ScriptObject* so I'm guessing additional protocol may be needed to handle that case (new tags, for sure).  However, if we are already handling toJSON methods on eg Number properly then that should be simple work.

Comment 7

[Felix S. Klock II [:pnkfelix, :fklock]]

(In reply to Lars T Hansen from comment #6)
> My concern is that a float4 value in the VM is not a ScriptObject* so I'm
> guessing additional protocol may be needed to handle that case (new tags,
> for sure).  However, if we are already handling toJSON methods on eg Number
> properly then that should be simple work.

Yeah I would d say we were going to have to do this anyway, assuming we want users to be able to override toJSON for Float4 the same way that they can override ByteArray.toJSON.

I assume that we want float values to just use the Number.toJSON, analogously to how I believe int and uint are handled.  But maybe this is a flawed assumption, since there are actually not int/uint values at runtime (just Numbers), while I assume there *are* float values at runtime, right?  So maybe there should be a Float.toJSON?

Comment 8

[Lars T Hansen]

For the record:

Discussed with Edwin and Felix whether to format a float4 as an array [x,y,z,w] or as a property list {"x":x, "y":y, "z":z, "w":z}.  General consensus is that the latter is more desirable.

Comment 9

[Lars T Hansen]

(In reply to Felix S Klock II from comment #7)
> (In reply to Lars T Hansen from comment #6)
> > My concern is that a float4 value in the VM is not a ScriptObject* so I'm
> > guessing additional protocol may be needed to handle that case (new tags,
> > for sure).  However, if we are already handling toJSON methods on eg Number
> > properly then that should be simple work.
> 
> Yeah I would d say we were going to have to do this anyway, assuming we want
> users to be able to override toJSON for Float4 the same way that they can
> override ByteArray.toJSON.

The apropriate analogous case for both float and float4 is Number, so we should try to keep the discussion to that.

> I assume that we want float values to just use the Number.toJSON,
> analogously to how I believe int and uint are handled.  But maybe this is a
> flawed assumption, since there are actually not int/uint values at runtime
> (just Numbers), while I assume there *are* float values at runtime, right? 
> So maybe there should be a Float.toJSON?

There are no int and uint values, but there are float and float4 values.  JSON stringification on float and float4 should behave as on Number, I think, despite the fact that the float4 case looks more complicated.  If the JSON case for Number can be overridden then the case for float and float4 should be overridable; if not, then not.

Comment 10

[Lars T Hansen]

Attachment: AS3 parts

On the high level: a float is a Number and should be treated that way; a float4 is not a structure and should not be subject to structure walking.

Comment 11

[Lars T Hansen]

Attachment: Proposed solution

Won't land until tr-float works with AVMFEATURE_FLOAT=0 and I can test that the code for those cases works as it should.

Felix, great if you have time for a quick look, but not a requirement.

Comment 12

[Felix S. Klock II [:pnkfelix, :fklock]]

Comment on attachment 580917 [details] [diff] [review]
Proposed solution

I had forgotten the mapping of non-finite values to "null".  Ah, JSON...

Considering your statement from comment 10 that "a float4 is not a structure and should not be subject to structure walking": I interpret JO as doing structure-walking (e.g. it does the JOProp loop, passing the key/value pairs to the replacer callback function, if any; see JSONSerializer::StrFoundValue).  Do you disagree?  (I am not saying that this traversal is wrong, in fact I think its fine; I am just pointing out a potential discrepancy in your patch and your comment.)

Comment 13

[Lars T Hansen]

(In reply to Felix S Klock II from comment #12)
> Comment on attachment 580917 [details] [diff] [review]
> Proposed solution
> 
> I had forgotten the mapping of non-finite values to "null".  Ah, JSON...
> 
> Considering your statement from comment 10 that "a float4 is not a structure
> and should not be subject to structure walking": I interpret JO as doing
> structure-walking (e.g. it does the JOProp loop, passing the key/value pairs
> to the replacer callback function, if any; see
> JSONSerializer::StrFoundValue).  Do you disagree?

No, I do not disagree.  The comment was wrong.

Comment 14

[Felix S. Klock II [:pnkfelix, :fklock]]

(In reply to Lars T Hansen from comment #9)
> If the
> JSON case for Number can be overridden then the case for float and float4
> should be overridable; if not, then not.

avmplus interactive shell
Type '?' for help

> JSON.stringify({x:43, y:44.4, z:new Number(45), w:uint(-46)})
{"x":43,"z":45,"w":4294967250,"y":44.4}
> Number.prototype.toJSON = function (k) { return this + 10; }
function Function() {}
> JSON.stringify({x:43, y:44.4, z:new Number(45), w:uint(-46)})
{"x":53,"z":55,"w":4294967260,"y":54.4}
> 

So yes, they should be overriddable.

(And we should have tests for this feature of the Number prototype, but I do not yet see evidence of any such tests in test/acceptance/ecma3/JSON/ )

Comment 15

[Lars T Hansen]

Core JSON code, a little different from the patch:

changeset:   7069:dab3767ce798
tag:         tip
user:        Lars T Hansen <[e-mail]>
date:        Tue Dec 13 13:25:47 2011 +0100
summary:     For 704071 - JSON output support for float and float4: core code, no test cases yet

What I have here appears to do rewriting properly: toJSON on float and float4 are invoked and their results obeyed.

Comment 16

[Lars T Hansen]

Test cases:

changeset:   7070:0d65da7a9abb
tag:         tip
user:        Lars T Hansen <[e-mail]>
date:        Tue Dec 13 14:13:53 2011 +0100
summary:     JSON on float/float4: test cases