Skip to content

Conversation

@bjornharrtell
Copy link
Collaborator

ref #6144

@bjornharrtell bjornharrtell changed the title WIP: [JS/TS] Rewrite flexbuffers JS to TS [JS/TS] Rewrite flexbuffers JS to TS Sep 27, 2020
@bjornharrtell
Copy link
Collaborator Author

@mzaks a bit tricky to port :) but I think this is close...

currently a curious bug causing test to fail at JavaScriptFlexBuffersTest.js:56 with:

actual: Uint8Array(29) [
  154, 153, 153, 153, 153, 153, 241,  63,
    0,   0,   0,   0,   0,   0,   0,   0,
    0, 255, 255, 255, 255, 255, 255, 255,
   15,   5,  18,  43,   1
],
expected: Uint8Array(29) [
    2,   0,   0,   0,   0,   0,   0,   0,
  154, 153, 153, 153, 153, 153, 241,  63,
    0, 255, 255, 255, 255, 255, 255, 255,
   15,   5,  18,  43,   1
],

@mzaks
Copy link
Contributor

mzaks commented Sep 28, 2020

It seems to be this assert:

_assert([1.1, -256.0], [2, 0, 0, 0, 0, 0, 0, 0, 154, 153, 153, 153, 153, 153, 241, 63, 0, 255, 255, 255, 255, 255, 255, 255, 15, 5, 18, 43, 1]);

On line 74. I would need some time to check out your branch and debug a bit :).
Could you please try to comment this one out and check if there are others which are failing?

@bjornharrtell
Copy link
Collaborator Author

Asserts at 81, 91, 265, 317 fails, and uncommenting that last one results in a thrown exception "Key [age] is not applicable on / of 9". I must have messed up something in the core logic.. don't expect you to debug it for me, but glad if you do: :)

@mzaks
Copy link
Contributor

mzaks commented Sep 28, 2020

@bjornharrtell I will try to take some time today in the evening.
Debugging binary formats is always fun 😀.

@mzaks
Copy link
Contributor

mzaks commented Sep 28, 2020

@bjornharrtell I found the bug.
builder.ts:28 should be this.builder.view.setFloat32(this.builder.offset, this.value as number, true);
builder.ts:30 should be this.builder.view.setFloat64(this.builder.offset, this.value as number, true);

You use this.offset instead of this.builder.offset

@bjornharrtell
Copy link
Collaborator Author

@mzaks nice catch. Fixed and went over code formatting.

@bjornharrtell bjornharrtell marked this pull request as ready for review September 28, 2020 14:39
@aardappel
Copy link
Collaborator

Nice! I'll merge once @mzaks approves.

@aardappel
Copy link
Collaborator

@krojew may want to have a look

@mzaks
Copy link
Contributor

mzaks commented Oct 19, 2020

I am super sorry for the long radio silence. I went through the code. It is good to be merged! And PR can be closed

@aardappel
Copy link
Collaborator

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants