Fix some warnings in the core library identified by the static analyzer.

Fix some warnings in the core library identified by the static analyzer.

This foray uncovered a latent bug in the byte array view constructor and
its use of default arguments.  That bug has been corrected and new tests
have been added to the test suite.

BUG=2136
Committed: https://code.google.com/p/dart/source/detail?r=9013

[gbracha, 2012-06-21 21:50:18]

lgtm

[siva, 2012-06-21 23:33:08]

lgtm with some comments.

http://codereview.chromium.org/10653002/diff/1/runtime/lib/byte_array.dart
File runtime/lib/byte_array.dart (right):

http://codereview.chromium.org/10653002/diff/1/runtime/lib/byte_array.dart#ne...
runtime/lib/byte_array.dart:901: length * this.bytesPerElement());
why not 
assert(this.bytesPerElement() === 1);
and leave it the way it was.

http://codereview.chromium.org/10653002/diff/1/runtime/lib/byte_array.dart#ne...
runtime/lib/byte_array.dart:2198: int setInt8(int byteOffset, int value) {
Why do these set methods not return void like the others and instead return int

http://codereview.chromium.org/10653002/diff/1/runtime/lib/byte_array.dart#ne...
runtime/lib/byte_array.dart:2913: }
This check code seems identical in each of the views why not make a method in
ByteArrayViewBase and invoke it from here.

http://codereview.chromium.org/10653002/diff/1/runtime/tests/vm/dart/byte_arr...
File runtime/tests/vm/dart/byte_array_test.dart (right):

http://codereview.chromium.org/10653002/diff/1/runtime/tests/vm/dart/byte_arr...
runtime/tests/vm/dart/byte_array_test.dart:937: Expect.isTrue(whole is
Int8List);
maybe a negative test too, e.g:

Expect.IsFalse(whole is Uint8List);

[cshapiro, 2012-06-22 04:27:00]

http://codereview.chromium.org/10653002/diff/1/runtime/lib/byte_array.dart
File runtime/lib/byte_array.dart (right):

http://codereview.chromium.org/10653002/diff/1/runtime/lib/byte_array.dart#ne...
runtime/lib/byte_array.dart:901: length * this.bytesPerElement());
Good question.  Keeping the code exactly the same between all types makes it
easier to write the emacs keyboard macros I use to make changes to all 20 cases.
 I'd like to keep this stuff as structurally similar as possible until that
becomes impossible to do.

http://codereview.chromium.org/10653002/diff/1/runtime/lib/byte_array.dart#ne...
runtime/lib/byte_array.dart:2198: int setInt8(int byteOffset, int value) {
Another good question!  This is actually documented in the interface. 
Basically, the amount of bytes displaced is returned so you can write code like
this

  pos += dst.setInt8(pos, x);
  pos += dst.setFloat64(pos, y);
  pos += dst.setUint16(pos, z);
 
when writing formatted binary data

http://codereview.chromium.org/10653002/diff/1/runtime/lib/byte_array.dart#ne...
runtime/lib/byte_array.dart:2913: }
Yes, this is doing pretty much what _rangeCheck already does!  So, I have
replaced this call with a call to _rangeCheck.

http://codereview.chromium.org/10653002/diff/1/runtime/tests/vm/dart/byte_arr...
File runtime/tests/vm/dart/byte_array_test.dart (right):

http://codereview.chromium.org/10653002/diff/1/runtime/tests/vm/dart/byte_arr...
runtime/tests/vm/dart/byte_array_test.dart:937: Expect.isTrue(whole is
Int8List);
Probably not a bad idea.  However, I do a lot of these tests so I should add
them in all places that I do a type test and that probably merits another
change.

Should I test that all other type tests answer false or just the unsigned
variant?  Let me know, I'll add this in another change set.

[siva, 2012-06-22 18:12:30]

lgtm

http://codereview.chromium.org/10653002/diff/1/runtime/lib/byte_array.dart
File runtime/lib/byte_array.dart (right):

http://codereview.chromium.org/10653002/diff/1/runtime/lib/byte_array.dart#ne...
runtime/lib/byte_array.dart:2198: int setInt8(int byteOffset, int value) {
dst.setInt8(pos, x); is equivalent to dst[pos] = x;

but 
pos += (dst[pos] = x);
is not quite the same, that is what makes it confusing 

On 2012/06/22 04:27:00, cshapiro wrote:
> Another good question!  This is actually documented in the interface. 
> Basically, the amount of bytes displaced is returned so you can write code
like
> this
> 
>   pos += dst.setInt8(pos, x);
>   pos += dst.setFloat64(pos, y);
>   pos += dst.setUint16(pos, z);
>  
> when writing formatted binary data

http://codereview.chromium.org/10653002/diff/1/runtime/tests/vm/dart/byte_arr...
File runtime/tests/vm/dart/byte_array_test.dart (right):

http://codereview.chromium.org/10653002/diff/1/runtime/tests/vm/dart/byte_arr...
runtime/tests/vm/dart/byte_array_test.dart:937: Expect.isTrue(whole is
Int8List);
No I meant just one negative test each just to ensure that the implementation
does not always return true.

On 2012/06/22 04:27:00, cshapiro wrote:
> Probably not a bad idea.  However, I do a lot of these tests so I should add
> them in all places that I do a type test and that probably merits another
> change.
> 
> Should I test that all other type tests answer false or just the unsigned
> variant?  Let me know, I'll add this in another change set.