Add DataView to standard library

[LinusU]

Opening this for early feedback, as soon as I know I'm on the right track it should be very easy for me to add all the required methods.

Some questions:

1. Should I just throw new RangeError('Out of bounds access') when trying to access outside the bounds?

2. Is there any way to overload the get* functions for when littleEndian is known at compile time, or will the compiler just figure it out anyway?

   e.g. something like this:

   function getUint32(offset: number, littleEndian: false) { ... }
   function getUint32(offset: number, littleEndian: true) { ... }
   function getUint32(offset: number, littleEndian: boolean) { ... }

3. Do I have to register DataView as a global variable somewhere?

4. Where do I put the tests? I'm assuming somewhere under tests/, but then I'm a bit lost ☺️

Thank you for your time, looking forward to working a lot more on this project, it rocks! 🙌

fixes #315

FIXME:

• Don't commit dist/* files
• Add me to NOTICE file

[MaxGraey]

Is there any way to overload the get* functions for when littleEndian is known at compile time, or will the compiler just figure it out anyway?

Don't worry about that. Binaryen has dae (dead argument elimination) and constant propagation if argument knows in compile time during optimization

[LinusU]

I will rebase and order/squash the commits in a good way when I'm done, just pushing now so that you can see the changes more easily 👍

[MaxGraey]

in AS better use var instead const if this variable can't compute in compile time or store in data section

[LinusU]

Good to know, will fix 👍

Do you mind explaining why? ☺️ it is also better than let?

[MaxGraey]

It better ask to @dcodeIO

[dcodeIO]

As a rule of thumb, const currently behaves pretty much like static const in other languages, indicating storage in static memory. I'm considering changing that, though.

[LinusU]

@dcodeIO Should we use let (unless const changes) then instead of var?

[MaxGraey]

About alignment. wasm can read/store unalignment memory without caveat:
WebAssembly/design#30 (comment)

[MaxGraey]

I don't think this support yet

[LinusU]

alas, it is not

ERROR TS2304: Cannot find name 'a'.

 function test (a: ArrayBuffer, l: i32 = a.byteLength): i32 {
                                         ~

Would it be hard to fix? 😁 maybe a bit out of scope for this PR though 😂

[LinusU]

diff --git a/std/assembly/dataview.ts b/std/assembly/dataview.ts
index f1a8ebe..ef5df88 100644
--- a/std/assembly/dataview.ts
+++ b/std/assembly/dataview.ts
@@ -16,8 +16,10 @@ export class DataView {
   constructor(
     readonly buffer: ArrayBuffer,
     readonly byteOffset: i32 = 0,
-    readonly byteLength: i32 = buffer.byteLength - byteOffset,
+    readonly byteLength: i32 = i32.MIN_VALUE,
   ) {
+    if (byteLength === i32.MIN_VALUE) byteLength = buffer.byteLength - byteOffset
+
     if (byteOffset < 0) throw new RangeError("byteOffset cannot be negative");
     if (byteLength < 0) throw new RangeError("byteLength cannot be negative");
     if (byteOffset + byteLength > buffer.byteLength) throw new RangeError("Length out of range of buffer");

Is this an acceptable compromise?

It won't give a proper error if someone passes in i32.MIN_VALUE as the byteLength, but I think we have to choose one i32 value to mean "nothing was passed"

(btw. creating a DataView with a byteLength of 0 is totally valid)

[MaxGraey]

What about this?

byteLength = min(buffer.byteLength - byteOffset, byteLength);

[LinusU]

I was thinking that it would be preferable to throw if a too long byteLength was passed, rather than silently truncating the length of the DataView. That is how the spec says as well:

https://www.ecma-international.org/ecma-262/6.0/#sec-dataview-buffer-byteoffset-bytelength

12.c. If offset+viewByteLength > bufferByteLength, throw a RangeError exception.

[LinusU]

Hmm, without fb34590 I'm getting the following error, would love some insight 🤔

ERROR AS200: Conversion from type 'i32' to 'u16' requires an explicit cast.

   return littleEndian ? result : bswap<T>(result);
                                           ~~~~~~
 in ~lib/dataview.ts(6,42)

ERROR AS200: Conversion from type 'i32' to 'u16' requires an explicit cast.

   return littleEndian ? result : bswap<T>(result);
          ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 in ~lib/dataview.ts(6,9)

[MaxGraey]

Also plz add tests to new file tests/compiler/std/dataview.ts

[LinusU]

I think that everything should be good to go now 🙌

I've rebased on master and squashed everything into one clean commit 😎

[MaxGraey]

Also need declare declare class DataView { .... } inside portable/index.d.ts. Just copy-paste this from assembly/index.d.ts

[LinusU]

Added entry to portable/index.d.ts and rebased on master again 👍

[dcodeIO]

Looking good to me, except that it somewhat feels like the get and set helpers aren't strictly necessary. Also, some methods use them, and others like getFloat32 don't?

[LinusU]

Yeah, they are currently only used for getting and setting integers, since floats require special handling. I'd be happy to either 1) remove the functions and just have the code at each callsite, 2) update them to also work for floats, so that they are used all the time or 3) rename them getInteger/setInteger or similar. Just let me know ☺️

[dcodeIO]

Regarding 2) I think that these could have been wrapped in a reinterpret instruction to make them work with no other changes, but I'd personally favor 1) to save the additional inlining step and resulting block in untouched output.

[LinusU]

Done 🚀

[dcodeIO]

Thank you! :)