Add endian parameter to the get/set functions in ByteData.

Add endian parameter to the get/set functions in ByteData.

Committed: https://code.google.com/p/dart/source/detail?r=21688

[vsm, 2013-04-18 04:06:56]

https://chromiumcodereview.appspot.com/14332002/diff/4001/sdk/lib/typeddata/t...
File sdk/lib/typeddata/typeddata.dart (right):

https://chromiumcodereview.appspot.com/14332002/diff/4001/sdk/lib/typeddata/t...
sdk/lib/typeddata/typeddata.dart:64: final bool little_endian;
Can we make this private?  Also, standard Dart style would be _littleEndian.

[Søren Gjesse, 2013-04-18 07:04:36]

dbc

https://chromiumcodereview.appspot.com/14332002/diff/4001/sdk/lib/typeddata/t...
File sdk/lib/typeddata/typeddata.dart (right):

https://chromiumcodereview.appspot.com/14332002/diff/4001/sdk/lib/typeddata/t...
sdk/lib/typeddata/typeddata.dart:161: int getInt16(int byteOffset, [Endianness
endian = Endianness.BIG_ENDIAN]);
How about adding a property endianess to ByteData to set the default endianess
used?

Then

ByteData bd = ...
db.endianess = Endianness.LITTLE_ENDIAN;
db.setInt16(1);
db.setInt32(2);
db.setInt64(1);

would be equal to

ByteData bd = ...
db.setInt16(1, Endianness.LITTLE_ENDIAN);
db.setInt32(2, Endianness.LITTLE_ENDIAN);
db.setInt64(1, Endianness.LITTLE_ENDIAN);

The default value for the endian argument should then just be null, and if null
pick the value from the endianess property which should default to
Endianness.BIG_ENDIAN.

When not using BIG_ENDIAN this would save writing Endianness.LITTLE_ENDIAN all
over the place.

[Mads Ager (google), 2013-04-18 08:10:44]

lgtm

https://chromiumcodereview.appspot.com/14332002/diff/4001/runtime/lib/typedda...
File runtime/lib/typeddata.cc (right):

https://chromiumcodereview.appspot.com/14332002/diff/4001/runtime/lib/typedda...
runtime/lib/typeddata.cc:328: return
Smi::New(Utils::HostToLittleEndian16(host_value.Value()));
Maybe extract host_value.Value() as you have done in the other ones?

https://chromiumcodereview.appspot.com/14332002/diff/4001/runtime/lib/typedda...
File runtime/lib/typeddata.dart (right):

https://chromiumcodereview.appspot.com/14332002/diff/4001/runtime/lib/typedda...
runtime/lib/typeddata.dart:3043: static int _ToEndianInt16(int host_value, bool
little_endian)
I think we usually start with a lower-case letter for the dart methods.
_toEndianXYZ.

https://chromiumcodereview.appspot.com/14332002/diff/4001/sdk/lib/typeddata/t...
File sdk/lib/typeddata/typeddata.dart (right):

https://chromiumcodereview.appspot.com/14332002/diff/4001/sdk/lib/typeddata/t...
sdk/lib/typeddata/typeddata.dart:56: const Endianness(this.little_endian);
Maybe we should make sure that the user cannot create extra values of this type
by making the constructor private?

const Endianness._(this._littleEndian);

Since the value is a boolean it doesn't really matter in this case though.

[siva, 2013-04-18 17:17:46]

https://chromiumcodereview.appspot.com/14332002/diff/4001/runtime/lib/typedda...
File runtime/lib/typeddata.cc (right):

https://chromiumcodereview.appspot.com/14332002/diff/4001/runtime/lib/typedda...
runtime/lib/typeddata.cc:328: return
Smi::New(Utils::HostToLittleEndian16(host_value.Value()));
On 2013/04/18 08:10:44, Mads Ager wrote:
> Maybe extract host_value.Value() as you have done in the other ones?

Done.

https://chromiumcodereview.appspot.com/14332002/diff/4001/runtime/lib/typedda...
File runtime/lib/typeddata.dart (right):

https://chromiumcodereview.appspot.com/14332002/diff/4001/runtime/lib/typedda...
runtime/lib/typeddata.dart:3043: static int _ToEndianInt16(int host_value, bool
little_endian)
On 2013/04/18 08:10:44, Mads Ager wrote:
> I think we usually start with a lower-case letter for the dart methods.
> _toEndianXYZ.

Done.

https://chromiumcodereview.appspot.com/14332002/diff/4001/sdk/lib/typeddata/t...
File sdk/lib/typeddata/typeddata.dart (right):

https://chromiumcodereview.appspot.com/14332002/diff/4001/sdk/lib/typeddata/t...
sdk/lib/typeddata/typeddata.dart:56: const Endianness(this.little_endian);
On 2013/04/18 08:10:44, Mads Ager wrote:
> Maybe we should make sure that the user cannot create extra values of this
type
> by making the constructor private?
> 
> const Endianness._(this._littleEndian);
> 
> Since the value is a boolean it doesn't really matter in this case though.

Done.

https://chromiumcodereview.appspot.com/14332002/diff/4001/sdk/lib/typeddata/t...
sdk/lib/typeddata/typeddata.dart:64: final bool little_endian;
On 2013/04/18 04:06:56, vsm wrote:
> Can we make this private?  Also, standard Dart style would be _littleEndian.

Done.

https://chromiumcodereview.appspot.com/14332002/diff/4001/sdk/lib/typeddata/t...
sdk/lib/typeddata/typeddata.dart:161: int getInt16(int byteOffset, [Endianness
endian = Endianness.BIG_ENDIAN]);
Seems like a good idea. I will defer this proposal to another CL. Maybe first
get feedback to see if dart2js will not have any issues implementing it this
way.

On 2013/04/18 07:04:36, Søren Gjesse wrote:
> How about adding a property endianess to ByteData to set the default endianess
> used?
> 
> Then
> 
> ByteData bd = ...
> db.endianess = Endianness.LITTLE_ENDIAN;
> db.setInt16(1);
> db.setInt32(2);
> db.setInt64(1);
> 
> would be equal to
> 
> ByteData bd = ...
> db.setInt16(1, Endianness.LITTLE_ENDIAN);
> db.setInt32(2, Endianness.LITTLE_ENDIAN);
> db.setInt64(1, Endianness.LITTLE_ENDIAN);
> 
> The default value for the endian argument should then just be null, and if
null
> pick the value from the endianess property which should default to
> Endianness.BIG_ENDIAN.
> 
> When not using BIG_ENDIAN this would save writing Endianness.LITTLE_ENDIAN all
> over the place.

[srdjan, 2013-04-18 17:32:33]

lgtm

[siva, 2013-04-18 18:19:50]

Committed patchset #3 manually as r21688 (presubmit successful).

[sra1, 2013-04-19 18:31:33]

https://codereview.chromium.org/14332002/diff/5011/sdk/lib/typeddata/typeddat...
File sdk/lib/typeddata/typeddata.dart (right):

https://codereview.chromium.org/14332002/diff/5011/sdk/lib/typeddata/typeddat...
sdk/lib/typeddata/typeddata.dart:59: static const Endianness LITTLE_ENDIAN =
const Endianness._(true);
These names are rather verbose and you can't write LITTLE_ENDIAN without the
class name Endianness:

bdata.getUint16(i, Endianness.LITTLE_ENDIAN)

How about removing some of the internal redundancy in the name to allow the user
to write:

bdata.getUint16(i, Endian.LITTLE)

https://codereview.chromium.org/14332002/diff/5011/sdk/lib/typeddata/typeddat...
sdk/lib/typeddata/typeddata.dart:146: * Throws [RangeError] if [byteOffset] is
negative,
This cannot be efficiently supported in dart2js.

JavaScript's DataView does range checking, but the error is a DOMException.

There is no way to convert the DOMException into another kind of exception - the
only information common across browsers is a localized message string.
Wrapping the call in try-catch, or doing an explicit range check, will lead to
performance not competitive with JavaScript.

Please change the documentation here to permit other exceptions, e.g

"Throws [RangeError] or [DOMException] if ..."

https://codereview.chromium.org/14332002/diff/5011/sdk/lib/typeddata/typeddat...
sdk/lib/typeddata/typeddata.dart:161: int getInt16(int byteOffset, [Endianness
endian = Endianness.BIG_ENDIAN]);
My recommendation at
https://code.google.com/p/dart/issues/detail?id=8220#c10
was to use a bool parameter 'littleEndian' that defaults to false for big
endian.

This recommendation allows the Dart code to compile directly to JavaScript
without needing to coax the compiler into achieving the required optimizations.

dart2js does not yet do all the optimizations necessary to reduce a Dart call
site data.getInt16(i) to the equivalent JavaScript code (i.e. data.getInt16(i)).