Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TypedArrays should support more than Double values #10

Open
niloc132 opened this issue Oct 9, 2017 · 6 comments
Open

TypedArrays should support more than Double values #10

niloc132 opened this issue Oct 9, 2017 · 6 comments

Comments

@niloc132
Copy link
Contributor

niloc132 commented Oct 9, 2017

From what I can tell in the integer_entities.txt update, the build now can generate int or Integer in some cases instead of Double. This doesn't seem to have been applied to the TypedArray subtypes, resulting in some unique getAt and setAt calls. Working from PlayN sources as I patch them to use elemental2 instead of JSOs, ByteBuffer.getShort now must look like this:

    public final short getShort (int baseOffset) {
      short bytes = 0;
      if (order == ByteOrder.BIG_ENDIAN) {
          bytes = (short)(((byte) (double) byteArray.getAt(baseOffset)) << 8);
          bytes |= (((byte) (double) byteArray.getAt(baseOffset + 1)) & 0xFF);
      } else {
          bytes = (short) (((byte) (double) byteArray.getAt(baseOffset + 1)) << 8);
          bytes |= (((byte) (double) byteArray.getAt(baseOffset)) & 0xFF);
      }
      return bytes;
    }

Since Int8Array is a TypedArray is a JsArrayLike<Double>, the getAt call returns Double, which cannot be shifted, so first we cast to double, which still can't be shifted, so cast to byte. Technically we could cast to int in the second one, but I wanted to be precise here. We of course have to assume that the browser would never return something outside the range of a byte here.

    public final ByteBuffer putInt (int baseOffset, int value) {
      if (order == ByteOrder.BIG_ENDIAN) {
          for (int i = 3; i >= 0; i--) {
              byteArray.setAt(baseOffset + i, (double)(byte)(value & 0xFF));
              value = value >> 8;
          }
      } else {
          for (int i = 0; i <= 3; i++) {
              byteArray.setAt(baseOffset + i, (double)(byte)(value & 0xFF));
              value = value >> 8;
          }
      }
      return this;
    }

Same idea here - the int value is being shifted and masked, and can't be used directly as a double. Granted, the cast to byte isn't technically required here, but it does seem less un-clear to a reader than casting an int to a double so you can put it in a byte array.

I'm not sure exactly how this can be improved, without just listing all of these signatures by hand, but this is pretty terrible Java to have to write. At least in the case of PlayN it is hidden behind emul code, but I had understood that elemental2 was meant to be user facing for the most part.

@niloc132
Copy link
Contributor Author

niloc132 commented Oct 9, 2017

If any chance is made here, the same should probably go for

  public Uint8Array(double[] length) {}

I do see that there is an integer_entities entry for

  public Uint8Array(double length) {}

But am confused if that will also cause the other constructor to use int[] (which at least is closer to byte[], but not quiiite right), or if it might assume that it should also be int.

@gkdn
Copy link
Member

gkdn commented Oct 9, 2017

Unfortunately we cannot make JsArrayLike since Integer is not a number but a boxed Java object.

The example you have is much better to be written as

bytes |= byteArray.getAnyAt(baseOffset + 1).asByte();

This is automatically runtime checked in debug mode for correctness. We can introduce JsArrayLikeInt and friend but I think the value add is much less compared to above example.

@jDramaix I think we can update double[] to be int[].

@niloc132
Copy link
Contributor Author

Thanks for the thought on asByte(), that will help a few cases of unboxing/casting. Doesn't help much for incoming values, or is there a "trust me, this is safe" cast for primitives? For setAt, it seems if those all took java.lang.Number instead (which IIRC is assumed to be js number, or is that only on the way out of native code?), we'd get the boxing for free.

@tbroyer
Copy link
Contributor

tbroyer commented Oct 10, 2017

java.lang.Number could be a Long or a BigDecimal or BigInteger, so cannot be used as an equivalent of JS Number.

@gkdn
Copy link
Member

gkdn commented Oct 10, 2017

jsinterop set methods has @DoNotAutobox that prevents autoboxing. The idea is a user dealing with jsinterop classes will not be looking for boxed objects. See following test:

  public void testSetAny() {
    JsArrayLike<Object> arrayLike = getArrayLikeOf();
    arrayLike.setAt(0, 15.5d);
    arrayLike.setAt(1, 15.5f);
    arrayLike.setAt(2, 15L);
    arrayLike.setAt(3, 15);
    arrayLike.setAt(4, (short) 15);
    arrayLike.setAt(5, (char) 15);
    arrayLike.setAt(6, (byte) 15);
    arrayLike.setAt(7, true);

    assertThat(arrayLike.getAnyAt(0).asDouble()).isEqualTo(15.5);
    assertThat(arrayLike.getAnyAt(1).asDouble()).isEqualTo(15.5);
    assertThat(arrayLike.getAnyAt(2).asLong()).isEqualTo(15L);
    assertThat(arrayLike.getAnyAt(3).asInt()).isEqualTo(15);
    assertThat(arrayLike.getAnyAt(4).asShort()).isEqualTo(15);
    assertThat(arrayLike.getAnyAt(5).asChar()).isEqualTo(15);
    assertThat(arrayLike.getAnyAt(6).asByte()).isEqualTo(15);
    assertThat(arrayLike.getAnyAt(7).asBoolean()).isTrue();
  }

@treblereel
Copy link
Contributor

it could be nice to have from(float[] source) method at Float32Array, atm it contains from(double[] source)

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

No branches or pull requests

4 participants