elemental2 icon indicating copy to clipboard operation
elemental2 copied to clipboard

TypedArrays should support more than `Double` values

Open niloc132 opened this issue 8 years ago • 6 comments

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 avatar Oct 09 '17 14:10 niloc132

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.

niloc132 avatar Oct 09 '17 14:10 niloc132

Unfortunately we cannot make JsArrayLike<Integer> 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[].

gkdn avatar Oct 09 '17 23:10 gkdn

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.

niloc132 avatar Oct 10 '17 16:10 niloc132

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

tbroyer avatar Oct 10 '17 17:10 tbroyer

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();
  }

gkdn avatar Oct 10 '17 23:10 gkdn

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

treblereel avatar Jun 07 '18 10:06 treblereel