Work in progres, please take a look and give early feedback if this is the way we want to structure…

Final version, this cl splits the Foreign interface into a set of new classes:
ForeignLibrary:
  Holds a pointer to an open library. The existing solution did not allow opening
  libraries not linked into the main program. We still allow using directly
  linked in libraries by using ForeignLibrary.main.lookup
ForeignPointer:
  Holds a foreign pointer
ForeignFunction:
  Hold a pointer to a foreign function, this is a subclass of ForeignPointer,
  and has all the functionality for calling functions.
  We still need a more generic solution for calling functions with arbritary
  arguments.
ForeignMemory:
  Holds a pointer to a piece of memory, and the lenght of the data. This is
  a subclass of ForeignPointer. This can also be instantiated from dart using
  the allocate and allocateFinalized.

This also adds some more testing, by adding a new c library that we compile and
use for validation of the functionality. This gives us the flexibility of
being more thorough in testing all our call and memory setting/getting functions.


Original working description for completeness.
Work in progres, please take a look and give early feedback if this is the way we want to structure this.

This is splitting the Foreign interface into several classes (maybe we should combine ForeignPointer and ForeignMemory).

It also adds some tests using a compiled c library that is part of this cl. We do this to actually test that we can use libraries not linked in and to better contoll what we get from there.

I still miss:
  Moving dart initiated allocation logic
  Remove old Foreign, and move the native functions up (but leaving them there for now allowed me to experiment with this before changing the IO logic)
  I have some more testing in mind.

BUG=
R=kasperl@google.com

Committed: https://github.com/dart-lang/fletch/commit/97002260e4d6cfcb4d0e0d82d4ae3ade07defb96

[ricow1, 2015-06-29 16:55:23]

ricow@google.com changed reviewers:
+ kasperl@google.com

[ricow1, 2015-06-29 16:55:24]

https://codereview.chromium.org/1209033003/diff/20001/lib/ffi/ffi.dart
File lib/ffi/ffi.dart (right):

https://codereview.chromium.org/1209033003/diff/20001/lib/ffi/ffi.dart#newcode12
lib/ffi/ffi.dart:12: class Foreign1 {
This will be renamed Foreign once we agree on the structure (and I will refactor
all the callsites)

https://codereview.chromium.org/1209033003/diff/20001/lib/ffi/ffi.dart#newcod...
lib/ffi/ffi.dart:173: => Foreign._getInt8(_computeAddress(offset, 1));
all the natives will move into this class

https://codereview.chromium.org/1209033003/diff/20001/src/vm/ffi_macos.cc
File src/vm/ffi_macos.cc (right):

https://codereview.chromium.org/1209033003/diff/20001/src/vm/ffi_macos.cc#new...
src/vm/ffi_macos.cc:9: const char* ForeignLibraryConstants::kLibBundlePrefix =
"foobar";
is probably not the right prefix, but fine for now :-)

https://codereview.chromium.org/1209033003/diff/20001/tests/ffi/ffi_test.dart
File tests/ffi/ffi_test.dart (right):

https://codereview.chromium.org/1209033003/diff/20001/tests/ffi/ffi_test.dart...
tests/ffi/ffi_test.dart:175: // TODO(ricow): Failure, need to investigate
this fails with arch ia32, still need to figure out why, could be me doing
something stupid

[ricow1, 2015-06-30 06:13:26]

Mac lib prefix/postfix

[Anders Johnsen, 2015-06-30 06:30:16]

ajohnsen@google.com changed reviewers:
+ ajohnsen@google.com

[Anders Johnsen, 2015-06-30 06:30:17]

DBC

https://codereview.chromium.org/1209033003/diff/40001/lib/ffi/ffi.dart
File lib/ffi/ffi.dart (right):

https://codereview.chromium.org/1209033003/diff/40001/lib/ffi/ffi.dart#newcode20
lib/ffi/ffi.dart:20: static const int LINUX   = 1;
We should use enums going forward.

https://codereview.chromium.org/1209033003/diff/40001/lib/ffi/ffi.dart#newcod...
lib/ffi/ffi.dart:166: class ForeignMemory extends ForeignPointer {
Should ForeignMemory have a way of auto-deleting (weak pointer based)?

https://codereview.chromium.org/1209033003/diff/40001/lib/ffi/ffi.dart#newcod...
lib/ffi/ffi.dart:226: if (_length > 0) Foreign._free(_value);
Consider using -1 for length, as malloc(0) may return a valid pointer.

[ricow1, 2015-06-30 06:52:07]

https://codereview.chromium.org/1209033003/diff/40001/lib/ffi/ffi.dart
File lib/ffi/ffi.dart (right):

https://codereview.chromium.org/1209033003/diff/40001/lib/ffi/ffi.dart#newcod...
lib/ffi/ffi.dart:166: class ForeignMemory extends ForeignPointer {
On 2015/06/30 06:30:17, Anders Johnsen wrote:
> Should ForeignMemory have a way of auto-deleting (weak pointer based)?

Yes, as stated in the review description I will move up the allocation logic,
and do something about auto cleanup

[kasperl, 2015-06-30 07:16:01]

First round of comments. I think you're on the right path!

https://codereview.chromium.org/1209033003/diff/40001/lib/ffi/ffi.dart
File lib/ffi/ffi.dart (right):

https://codereview.chromium.org/1209033003/diff/40001/lib/ffi/ffi.dart#newcode14
lib/ffi/ffi.dart:14: int get value => _value;
I guess it will be nice when you can stop exposing this and start exposing
something that make more sense (address for ForeignMemory, etc.).

https://codereview.chromium.org/1209033003/diff/40001/lib/ffi/ffi.dart#newcode38
lib/ffi/ffi.dart:38: class ForeignFunction extends Foreign1 {
So to call a method that returns a C function pointer you have to do something
like this:
 
  var ptr = fun.pcall$0(new ForeignPointer());
  var moreFun = new ForeignFunction.fromAddress(ptr.value);

Should ForeignFunction be a ForeignPointer? That way you could write:

  var moreFun = fun.pcall$0(new ForeignFunction());

?

https://codereview.chromium.org/1209033003/diff/40001/lib/ffi/ffi.dart#newcode95
lib/ffi/ffi.dart:95: int Lcall$wLw(a0, a1, a2) {
What's the purpose of this? Is it because you don't want to "split" on the word
size and have to different implementations of this?

I think adding support for getting 64-bit integers back (something ala
lcall$...) would make sense, but I guess I dislike encoding too much about the
arguments in the signature if we can avoid it.

https://codereview.chromium.org/1209033003/diff/40001/lib/ffi/ffi.dart#newcod...
lib/ffi/ffi.dart:128: static ForeignFunction lookupDefaultLibraries(String name)
{
Maybe it would be cleaner to have a ForeignLibrary.default so you would write:

  ForeignLibrary.default.lookupFunction(...)

instead of:

  ForeignLibrary.lookupDefaultLibraries(...)

?

https://codereview.chromium.org/1209033003/diff/40001/lib/ffi/ffi.dart#newcod...
lib/ffi/ffi.dart:136: static String bundleLibraryName(String libraryName) =>
Why do you need the separate _bundlePath function? Can't bundleLibraryName just
be the native one?

https://codereview.chromium.org/1209033003/diff/40001/lib/ffi/ffi.dart#newcod...
lib/ffi/ffi.dart:136: static String bundleLibraryName(String libraryName) =>
Add a comment that explains what bundleLibraryName is.

https://codereview.chromium.org/1209033003/diff/40001/lib/ffi/ffi.dart#newcod...
lib/ffi/ffi.dart:166: class ForeignMemory extends ForeignPointer {
Could and should this implement some typed_data interface at some point
(ByteData?). Not sure.

https://codereview.chromium.org/1209033003/diff/40001/lib/ffi/ffi.dart#newcod...
lib/ffi/ffi.dart:169: ForeignMemory.fromForeignPointer(ForeignPointer pointer,
int this._length) :
No need to type the this._length parameter. The language guarantees that the
static analyzer automatically infers this.

https://codereview.chromium.org/1209033003/diff/40001/lib/ffi/ffi.dart#newcod...
lib/ffi/ffi.dart:219: // address for memory address functionality.
derived address -> derived addresses?

https://codereview.chromium.org/1209033003/diff/40001/src/vm/ffi.h
File src/vm/ffi.h (right):

https://codereview.chromium.org/1209033003/diff/40001/src/vm/ffi.h#newcode28
src/vm/ffi.h:28: class FFIUtils {
I'd prefer ForeignUtils, I think.

https://codereview.chromium.org/1209033003/diff/40001/src/vm/ffi.h#newcode34
src/vm/ffi.h:34: static char* DirName(char* path);
DirectoryName?

https://codereview.chromium.org/1209033003/diff/40001/src/vm/ffi_test_library.c
File src/vm/ffi_test_library.c (right):

https://codereview.chromium.org/1209033003/diff/40001/src/vm/ffi_test_library...
src/vm/ffi_test_library.c:52: int icall0() {
Maybe replace call with fun in all these methods to avoid confusion?

https://codereview.chromium.org/1209033003/diff/40001/src/vm/ffi_test_library...
src/vm/ffi_test_library.c:192: float* data =  malloc(sizeof(float) * 4);
=  malloc -> = malloc

https://codereview.chromium.org/1209033003/diff/40001/src/vm/ffi_test_library...
src/vm/ffi_test_library.c:200: void* memFloat64() {
Float -> float?

https://codereview.chromium.org/1209033003/diff/40001/src/vm/ffi_test_library...
src/vm/ffi_test_library.c:201: double* data =  malloc(sizeof(double) * 4);
=  malloc -> = malloc

https://codereview.chromium.org/1209033003/diff/40001/tests/ffi/ffi_test.dart
File tests/ffi/ffi_test.dart (right):

https://codereview.chromium.org/1209033003/diff/40001/tests/ffi/ffi_test.dart...
tests/ffi/ffi_test.dart:20: // NEW
Remove // NEW comment.

https://codereview.chromium.org/1209033003/diff/40001/tests/ffi/ffi_test.dart...
tests/ffi/ffi_test.dart:43: // Access memory out of bounds
.

https://codereview.chromium.org/1209033003/diff/40001/tests/ffi/ffi_test.dart...
tests/ffi/ffi_test.dart:46: (e) => e is NoSuchMethodError);
Is this really the error we should get?

https://codereview.chromium.org/1209033003/diff/40001/tests/ffi/ffi_test.dart...
tests/ffi/ffi_test.dart:102: // Access memory out of bounds
.

https://codereview.chromium.org/1209033003/diff/40001/tests/ffi/ffi_test.dart...
tests/ffi/ffi_test.dart:105: (e) => e is NoSuchMethodError);
Is this really the error we should get?

https://codereview.chromium.org/1209033003/diff/40001/tests/ffi/ffi_test.dart...
tests/ffi/ffi_test.dart:121: // Access memory out of bounds
.

https://codereview.chromium.org/1209033003/diff/40001/tests/ffi/ffi_test.dart...
tests/ffi/ffi_test.dart:124: (e) => e is NoSuchMethodError);
Is this really the error we should get?

https://codereview.chromium.org/1209033003/diff/40001/tests/ffi/ffi_test.dart...
tests/ffi/ffi_test.dart:140: // Access memory out of bounds
Maybe add a helper for this kind of check, so I don't have to repeat my comments
so many times.

https://codereview.chromium.org/1209033003/diff/40001/tests/ffi/ffi_test.dart...
tests/ffi/ffi_test.dart:315: /// OLD
Is this something you want to get rid of?

[ricow1, 2015-06-30 07:34:23]

Thank you both for comments. I will incorporate them, refactor IO to use the new
interface, and upload a new version for full review.

https://codereview.chromium.org/1209033003/diff/40001/lib/ffi/ffi.dart
File lib/ffi/ffi.dart (right):

https://codereview.chromium.org/1209033003/diff/40001/lib/ffi/ffi.dart#newcode14
lib/ffi/ffi.dart:14: int get value => _value;
On 2015/06/30 07:16:00, kasperl wrote:
> I guess it will be nice when you can stop exposing this and start exposing
> something that make more sense (address for ForeignMemory, etc.).

yes

https://codereview.chromium.org/1209033003/diff/40001/lib/ffi/ffi.dart#newcode20
lib/ffi/ffi.dart:20: static const int LINUX   = 1;
On 2015/06/30 06:30:17, Anders Johnsen wrote:
> We should use enums going forward.

Acknowledged.

https://codereview.chromium.org/1209033003/diff/40001/lib/ffi/ffi.dart#newcode38
lib/ffi/ffi.dart:38: class ForeignFunction extends Foreign1 {
On 2015/06/30 07:16:00, kasperl wrote:
> So to call a method that returns a C function pointer you have to do something
> like this:
>  
>   var ptr = fun.pcall$0(new ForeignPointer());
>   var moreFun = new ForeignFunction.fromAddress(ptr.value);
> 
> Should ForeignFunction be a ForeignPointer? That way you could write:
> 
>   var moreFun = fun.pcall$0(new ForeignFunction());
> 
> ?

Good point, will do that

https://codereview.chromium.org/1209033003/diff/40001/lib/ffi/ffi.dart#newcode95
lib/ffi/ffi.dart:95: int Lcall$wLw(a0, a1, a2) {
On 2015/06/30 07:16:00, kasperl wrote:
> What's the purpose of this? Is it because you don't want to "split" on the
word
> size and have to different implementations of this?
> 
> I think adding support for getting 64-bit integers back (something ala
> lcall$...) would make sense, but I guess I dislike encoding too much about the
> arguments in the signature if we can avoid it.

This is just an existing function, used for rseek. I discussed this a bit with
ajohnsen and sgjesse this morning. We could do something way more generic, where
you basically push the arguments for more specialized functions like this
(initialize with the number of arguments, something like (with better name):
FlexibleForeignFunction f = new FlexibleForeignFunction(numargs: 3);
f.addInt64(a)..addWord(b)..addInt8(c);
f.call();
Then we create the (platform specific) calling code for this.
We can't possibly support all permutations of functions like this.

(the above is just an example, maybe we should initialize it with the types
instead of the number of args)

https://codereview.chromium.org/1209033003/diff/40001/lib/ffi/ffi.dart#newcod...
lib/ffi/ffi.dart:128: static ForeignFunction lookupDefaultLibraries(String name)
{
On 2015/06/30 07:16:00, kasperl wrote:
> Maybe it would be cleaner to have a ForeignLibrary.default so you would write:
> 
>   ForeignLibrary.default.lookupFunction(...)
> 
> instead of:
> 
>   ForeignLibrary.lookupDefaultLibraries(...)
> 
> ?

sgtm

https://codereview.chromium.org/1209033003/diff/40001/lib/ffi/ffi.dart#newcod...
lib/ffi/ffi.dart:136: static String bundleLibraryName(String libraryName) =>
On 2015/06/30 07:16:00, kasperl wrote:
> Add a comment that explains what bundleLibraryName is.

Acknowledged.

https://codereview.chromium.org/1209033003/diff/40001/lib/ffi/ffi.dart#newcod...
lib/ffi/ffi.dart:136: static String bundleLibraryName(String libraryName) =>
On 2015/06/30 07:16:00, kasperl wrote:
> Why do you need the separate _bundlePath function? Can't bundleLibraryName
just
> be the native one?

Acknowledged.

https://codereview.chromium.org/1209033003/diff/40001/lib/ffi/ffi.dart#newcod...
lib/ffi/ffi.dart:166: class ForeignMemory extends ForeignPointer {
On 2015/06/30 07:16:00, kasperl wrote:
> Could and should this implement some typed_data interface at some point
> (ByteData?). Not sure.

Yes I probably could, I will look into it

https://codereview.chromium.org/1209033003/diff/40001/lib/ffi/ffi.dart#newcod...
lib/ffi/ffi.dart:169: ForeignMemory.fromForeignPointer(ForeignPointer pointer,
int this._length) :
On 2015/06/30 07:16:00, kasperl wrote:
> No need to type the this._length parameter. The language guarantees that the
> static analyzer automatically infers this.

Acknowledged.

https://codereview.chromium.org/1209033003/diff/40001/lib/ffi/ffi.dart#newcod...
lib/ffi/ffi.dart:219: // address for memory address functionality.
On 2015/06/30 07:16:00, kasperl wrote:
> derived address -> derived addresses?

Acknowledged.

https://codereview.chromium.org/1209033003/diff/40001/lib/ffi/ffi.dart#newcod...
lib/ffi/ffi.dart:226: if (_length > 0) Foreign._free(_value);
On 2015/06/30 06:30:17, Anders Johnsen wrote:
> Consider using -1 for length, as malloc(0) may return a valid pointer.

Acknowledged.

https://codereview.chromium.org/1209033003/diff/40001/src/vm/ffi.h
File src/vm/ffi.h (right):

https://codereview.chromium.org/1209033003/diff/40001/src/vm/ffi.h#newcode28
src/vm/ffi.h:28: class FFIUtils {
On 2015/06/30 07:16:00, kasperl wrote:
> I'd prefer ForeignUtils, I think.

Acknowledged.

https://codereview.chromium.org/1209033003/diff/40001/src/vm/ffi.h#newcode34
src/vm/ffi.h:34: static char* DirName(char* path);
On 2015/06/30 07:16:00, kasperl wrote:
> DirectoryName?

Acknowledged.

https://codereview.chromium.org/1209033003/diff/40001/src/vm/ffi_test_library.c
File src/vm/ffi_test_library.c (right):

https://codereview.chromium.org/1209033003/diff/40001/src/vm/ffi_test_library...
src/vm/ffi_test_library.c:52: int icall0() {
On 2015/06/30 07:16:01, kasperl wrote:
> Maybe replace call with fun in all these methods to avoid confusion?

Acknowledged.

https://codereview.chromium.org/1209033003/diff/40001/src/vm/ffi_test_library...
src/vm/ffi_test_library.c:192: float* data =  malloc(sizeof(float) * 4);
On 2015/06/30 07:16:00, kasperl wrote:
> =  malloc -> = malloc

this was clearly just a test to see if your whitespace sensitivity was still
intact :-)

https://codereview.chromium.org/1209033003/diff/40001/src/vm/ffi_test_library...
src/vm/ffi_test_library.c:200: void* memFloat64() {
On 2015/06/30 07:16:01, kasperl wrote:
> Float -> float?

Acknowledged.

https://codereview.chromium.org/1209033003/diff/40001/src/vm/ffi_test_library...
src/vm/ffi_test_library.c:201: double* data =  malloc(sizeof(double) * 4);
On 2015/06/30 07:16:00, kasperl wrote:
> =  malloc -> = malloc

Acknowledged.

https://codereview.chromium.org/1209033003/diff/40001/tests/ffi/ffi_test.dart
File tests/ffi/ffi_test.dart (right):

https://codereview.chromium.org/1209033003/diff/40001/tests/ffi/ffi_test.dart...
tests/ffi/ffi_test.dart:20: // NEW
On 2015/06/30 07:16:01, kasperl wrote:
> Remove // NEW comment.

Acknowledged.

https://codereview.chromium.org/1209033003/diff/40001/tests/ffi/ffi_test.dart...
tests/ffi/ffi_test.dart:43: // Access memory out of bounds
On 2015/06/30 07:16:01, kasperl wrote:
> .

Acknowledged.

https://codereview.chromium.org/1209033003/diff/40001/tests/ffi/ffi_test.dart...
tests/ffi/ffi_test.dart:46: (e) => e is NoSuchMethodError);
On 2015/06/30 07:16:01, kasperl wrote:
> Is this really the error we should get?

No, see comment below

https://codereview.chromium.org/1209033003/diff/40001/tests/ffi/ffi_test.dart...
tests/ffi/ffi_test.dart:102: // Access memory out of bounds
On 2015/06/30 07:16:01, kasperl wrote:
> .

Acknowledged.

https://codereview.chromium.org/1209033003/diff/40001/tests/ffi/ffi_test.dart...
tests/ffi/ffi_test.dart:105: (e) => e is NoSuchMethodError);
On 2015/06/30 07:16:01, kasperl wrote:
> Is this really the error we should get?

See comment below

https://codereview.chromium.org/1209033003/diff/40001/tests/ffi/ffi_test.dart...
tests/ffi/ffi_test.dart:121: // Access memory out of bounds
On 2015/06/30 07:16:01, kasperl wrote:
> .

Acknowledged.

https://codereview.chromium.org/1209033003/diff/40001/tests/ffi/ffi_test.dart...
tests/ffi/ffi_test.dart:124: (e) => e is NoSuchMethodError);
On 2015/06/30 07:16:01, kasperl wrote:
> Is this really the error we should get?

No it is not, I will make the the native code catch the real one, but I did not
want to touch the Foreign class implementation before I knew  if we agreed on
the interface

https://codereview.chromium.org/1209033003/diff/40001/tests/ffi/ffi_test.dart...
tests/ffi/ffi_test.dart:140: // Access memory out of bounds
On 2015/06/30 07:16:01, kasperl wrote:
> Maybe add a helper for this kind of check, so I don't have to repeat my
comments
> so many times.

Acknowledged.

https://codereview.chromium.org/1209033003/diff/40001/tests/ffi/ffi_test.dart...
tests/ffi/ffi_test.dart:315: /// OLD
On 2015/06/30 07:16:01, kasperl wrote:
> Is this something you want to get rid of?

Yep, and all of the below

[ahe, 2015-06-30 13:16:51]

https://codereview.chromium.org/1209033003/diff/40001/lib/ffi/ffi.dart
File lib/ffi/ffi.dart (right):

https://codereview.chromium.org/1209033003/diff/40001/lib/ffi/ffi.dart#newcode12
lib/ffi/ffi.dart:12: class Foreign1 {
Can I assume that you're planning on deleting Foreign and that this class will
be renamed to Foreign?

If so, perhaps you could just do that. It would make the review easier to read,
IMO.

[ricow1, 2015-06-30 19:26:27]

On 2015/06/30 13:16:51, ahe wrote:
> https://codereview.chromium.org/1209033003/diff/40001/lib/ffi/ffi.dart
> File lib/ffi/ffi.dart (right):
> 
>
https://codereview.chromium.org/1209033003/diff/40001/lib/ffi/ffi.dart#newcode12
> lib/ffi/ffi.dart:12: class Foreign1 {
> Can I assume that you're planning on deleting Foreign and that this class will
> be renamed to Foreign?
> 
> If so, perhaps you could just do that. It would make the review easier to
read,
> IMO.
That is explicitly described in the description of the cl! It states that this
is a request for comments on how to structure the api, before I go change all
the call sites in IO code, and that the Foreign1 class will be renamed Foreign,
and that the native methods will be moved up to this new class.

[ahe, 2015-06-30 19:56:59]

On 2015/06/30 19:26:27, ricow1 wrote:
> On 2015/06/30 13:16:51, ahe wrote:
> > https://codereview.chromium.org/1209033003/diff/40001/lib/ffi/ffi.dart
> > File lib/ffi/ffi.dart (right):
> > 
> >
>
https://codereview.chromium.org/1209033003/diff/40001/lib/ffi/ffi.dart#newcode12
> > lib/ffi/ffi.dart:12: class Foreign1 {
> > Can I assume that you're planning on deleting Foreign and that this class
will
> > be renamed to Foreign?
> > 
> > If so, perhaps you could just do that. It would make the review easier to
> read,
> > IMO.
> That is explicitly described in the description of the cl! It states that this
> is a request for comments on how to structure the api, before I go change all
> the call sites in IO code, and that the Foreign1 class will be renamed
Foreign,
> and that the native methods will be moved up to this new class.

Regardless, I'm having a hard time looking at this in its current form, so won't
be able to provide much feedback.

[ricow1, 2015-07-01 04:32:39]

On 2015/06/30 19:56:59, ahe wrote:
> On 2015/06/30 19:26:27, ricow1 wrote:
> > On 2015/06/30 13:16:51, ahe wrote:
> > > https://codereview.chromium.org/1209033003/diff/40001/lib/ffi/ffi.dart
> > > File lib/ffi/ffi.dart (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1209033003/diff/40001/lib/ffi/ffi.dart#newcode12
> > > lib/ffi/ffi.dart:12: class Foreign1 {
> > > Can I assume that you're planning on deleting Foreign and that this class
> will
> > > be renamed to Foreign?
> > > 
> > > If so, perhaps you could just do that. It would make the review easier to
> > read,
> > > IMO.
> > That is explicitly described in the description of the cl! It states that
this
> > is a request for comments on how to structure the api, before I go change
all
> > the call sites in IO code, and that the Foreign1 class will be renamed
> Foreign,
> > and that the native methods will be moved up to this new class.
> 
> Regardless, I'm having a hard time looking at this in its current form, so
won't
> be able to provide much feedback.

That is perfectly fine, if you go ahead and read the message I sent here a few
hours before you got involved you can see that I am now working on getting the
full version ready, copied here for your convenience:
Thank you both for comments. I will incorporate them, refactor IO to use the new
interface, and upload a new version for full review

[ricow1, 2015-07-02 16:01:57]

PTAL

https://codereview.chromium.org/1209033003/diff/40001/lib/ffi/ffi.dart
File lib/ffi/ffi.dart (right):

https://codereview.chromium.org/1209033003/diff/40001/lib/ffi/ffi.dart#newcode12
lib/ffi/ffi.dart:12: class Foreign1 {
On 2015/06/30 13:16:50, ahe wrote:
> Can I assume that you're planning on deleting Foreign and that this class will
> be renamed to Foreign?
> 
> If so, perhaps you could just do that. It would make the review easier to
read,
> IMO.

Done.

https://codereview.chromium.org/1209033003/diff/40001/lib/ffi/ffi.dart#newcode14
lib/ffi/ffi.dart:14: int get value => _value;
On 2015/06/30 07:16:00, kasperl wrote:
> I guess it will be nice when you can stop exposing this and start exposing
> something that make more sense (address for ForeignMemory, etc.).

Done.

https://codereview.chromium.org/1209033003/diff/40001/lib/ffi/ffi.dart#newcode38
lib/ffi/ffi.dart:38: class ForeignFunction extends Foreign1 {
On 2015/06/30 07:34:21, ricow1 wrote:
> On 2015/06/30 07:16:00, kasperl wrote:
> > So to call a method that returns a C function pointer you have to do
something
> > like this:
> >  
> >   var ptr = fun.pcall$0(new ForeignPointer());
> >   var moreFun = new ForeignFunction.fromAddress(ptr.value);
> > 
> > Should ForeignFunction be a ForeignPointer? That way you could write:
> > 
> >   var moreFun = fun.pcall$0(new ForeignFunction());
> > 
> > ?
> 
> Good point, will do that

Done.

https://codereview.chromium.org/1209033003/diff/40001/lib/ffi/ffi.dart#newcod...
lib/ffi/ffi.dart:128: static ForeignFunction lookupDefaultLibraries(String name)
{
On 2015/06/30 07:34:21, ricow1 wrote:
> On 2015/06/30 07:16:00, kasperl wrote:
> > Maybe it would be cleaner to have a ForeignLibrary.default so you would
write:
> > 
> >   ForeignLibrary.default.lookupFunction(...)
> > 
> > instead of:
> > 
> >   ForeignLibrary.lookupDefaultLibraries(...)
> > 
> > ?
> 
> sgtm

default is a reserved word - I called it standard now, but better alternatives
are welcome

https://codereview.chromium.org/1209033003/diff/40001/lib/ffi/ffi.dart#newcod...
lib/ffi/ffi.dart:136: static String bundleLibraryName(String libraryName) =>
On 2015/06/30 07:34:21, ricow1 wrote:
> On 2015/06/30 07:16:00, kasperl wrote:
> > Add a comment that explains what bundleLibraryName is.
> 
> Acknowledged.

Done.

https://codereview.chromium.org/1209033003/diff/40001/lib/ffi/ffi.dart#newcod...
lib/ffi/ffi.dart:169: ForeignMemory.fromForeignPointer(ForeignPointer pointer,
int this._length) :
On 2015/06/30 07:34:21, ricow1 wrote:
> On 2015/06/30 07:16:00, kasperl wrote:
> > No need to type the this._length parameter. The language guarantees that the
> > static analyzer automatically infers this.
> 
> Acknowledged.

Done.

https://codereview.chromium.org/1209033003/diff/40001/lib/ffi/ffi.dart#newcod...
lib/ffi/ffi.dart:219: // address for memory address functionality.
On 2015/06/30 07:34:21, ricow1 wrote:
> On 2015/06/30 07:16:00, kasperl wrote:
> > derived address -> derived addresses?
> 
> Acknowledged.

Done.

https://codereview.chromium.org/1209033003/diff/40001/src/vm/ffi.h
File src/vm/ffi.h (right):

https://codereview.chromium.org/1209033003/diff/40001/src/vm/ffi.h#newcode28
src/vm/ffi.h:28: class FFIUtils {
On 2015/06/30 07:34:22, ricow1 wrote:
> On 2015/06/30 07:16:00, kasperl wrote:
> > I'd prefer ForeignUtils, I think.
> 
> Acknowledged.

Done.

https://codereview.chromium.org/1209033003/diff/40001/src/vm/ffi.h#newcode34
src/vm/ffi.h:34: static char* DirName(char* path);
On 2015/06/30 07:34:22, ricow1 wrote:
> On 2015/06/30 07:16:00, kasperl wrote:
> > DirectoryName?
> 
> Acknowledged.

Done.

https://codereview.chromium.org/1209033003/diff/40001/src/vm/ffi_test_library.c
File src/vm/ffi_test_library.c (right):

https://codereview.chromium.org/1209033003/diff/40001/src/vm/ffi_test_library...
src/vm/ffi_test_library.c:52: int icall0() {
On 2015/06/30 07:34:22, ricow1 wrote:
> On 2015/06/30 07:16:01, kasperl wrote:
> > Maybe replace call with fun in all these methods to avoid confusion?
> 
> Acknowledged.

Done.

https://codereview.chromium.org/1209033003/diff/40001/src/vm/ffi_test_library...
src/vm/ffi_test_library.c:192: float* data =  malloc(sizeof(float) * 4);
On 2015/06/30 07:34:22, ricow1 wrote:
> On 2015/06/30 07:16:00, kasperl wrote:
> > =  malloc -> = malloc
> 
> this was clearly just a test to see if your whitespace sensitivity was still
> intact :-)

Done.

https://codereview.chromium.org/1209033003/diff/40001/src/vm/ffi_test_library...
src/vm/ffi_test_library.c:200: void* memFloat64() {
On 2015/06/30 07:34:22, ricow1 wrote:
> On 2015/06/30 07:16:01, kasperl wrote:
> > Float -> float?
> 
> Acknowledged.

Done.

https://codereview.chromium.org/1209033003/diff/40001/src/vm/ffi_test_library...
src/vm/ffi_test_library.c:201: double* data =  malloc(sizeof(double) * 4);
On 2015/06/30 07:34:22, ricow1 wrote:
> On 2015/06/30 07:16:00, kasperl wrote:
> > =  malloc -> = malloc
> 
> Acknowledged.

Done.

https://codereview.chromium.org/1209033003/diff/40001/tests/ffi/ffi_test.dart
File tests/ffi/ffi_test.dart (right):

https://codereview.chromium.org/1209033003/diff/40001/tests/ffi/ffi_test.dart...
tests/ffi/ffi_test.dart:20: // NEW
On 2015/06/30 07:34:22, ricow1 wrote:
> On 2015/06/30 07:16:01, kasperl wrote:
> > Remove // NEW comment.
> 
> Acknowledged.

Done.

https://codereview.chromium.org/1209033003/diff/40001/tests/ffi/ffi_test.dart...
tests/ffi/ffi_test.dart:43: // Access memory out of bounds
On 2015/06/30 07:34:22, ricow1 wrote:
> On 2015/06/30 07:16:01, kasperl wrote:
> > .
> 
> Acknowledged.

Done.

https://codereview.chromium.org/1209033003/diff/40001/tests/ffi/ffi_test.dart...
tests/ffi/ffi_test.dart:46: (e) => e is NoSuchMethodError);
On 2015/06/30 07:34:22, ricow1 wrote:
> On 2015/06/30 07:16:01, kasperl wrote:
> > Is this really the error we should get?
> 
> No, see comment below

Done.

https://codereview.chromium.org/1209033003/diff/40001/tests/ffi/ffi_test.dart...
tests/ffi/ffi_test.dart:102: // Access memory out of bounds
On 2015/06/30 07:34:22, ricow1 wrote:
> On 2015/06/30 07:16:01, kasperl wrote:
> > .
> 
> Acknowledged.

Done.

https://codereview.chromium.org/1209033003/diff/40001/tests/ffi/ffi_test.dart...
tests/ffi/ffi_test.dart:105: (e) => e is NoSuchMethodError);
On 2015/06/30 07:34:22, ricow1 wrote:
> On 2015/06/30 07:16:01, kasperl wrote:
> > Is this really the error we should get?
> 
> See comment below

Done.

https://codereview.chromium.org/1209033003/diff/40001/tests/ffi/ffi_test.dart...
tests/ffi/ffi_test.dart:121: // Access memory out of bounds
On 2015/06/30 07:34:22, ricow1 wrote:
> On 2015/06/30 07:16:01, kasperl wrote:
> > .
> 
> Acknowledged.

Done.

https://codereview.chromium.org/1209033003/diff/40001/tests/ffi/ffi_test.dart...
tests/ffi/ffi_test.dart:124: (e) => e is NoSuchMethodError);
On 2015/06/30 07:34:22, ricow1 wrote:
> On 2015/06/30 07:16:01, kasperl wrote:
> > Is this really the error we should get?
> 
> No it is not, I will make the the native code catch the real one, but I did
not
> want to touch the Foreign class implementation before I knew  if we agreed on
> the interface

Done.

https://codereview.chromium.org/1209033003/diff/40001/tests/ffi/ffi_test.dart...
tests/ffi/ffi_test.dart:140: // Access memory out of bounds
On 2015/06/30 07:34:23, ricow1 wrote:
> On 2015/06/30 07:16:01, kasperl wrote:
> > Maybe add a helper for this kind of check, so I don't have to repeat my
> comments
> > so many times.
> 
> Acknowledged.

Done.

https://codereview.chromium.org/1209033003/diff/40001/tests/ffi/ffi_test.dart...
tests/ffi/ffi_test.dart:315: /// OLD
On 2015/06/30 07:34:22, ricow1 wrote:
> On 2015/06/30 07:16:01, kasperl wrote:
> > Is this something you want to get rid of?
> 
> Yep, and all of the below

Done.

https://codereview.chromium.org/1209033003/diff/60001/src/vm/ffi.cc
File src/vm/ffi.cc (right):

https://codereview.chromium.org/1209033003/diff/60001/src/vm/ffi.cc#newcode462
src/vm/ffi.cc:462: if (value->IsSmi()) {                                        
         \
This already landed in another cl, I redid this here to pass my tests, but I did
not want to rebase before getting the review done (since the diff would include
a lot of stuff)

https://codereview.chromium.org/1209033003/diff/60001/src/vm/object.h
File src/vm/object.h (right):

https://codereview.chromium.org/1209033003/diff/60001/src/vm/object.h#newcode179
src/vm/object.h:179: FOREIGNFUNCTION_MARKER  = 5,
I know to little about our structure to know if we really need this?, but I did
not see a way to create the Instances programatically without the split (since
one has one instance variable, the other two)

[kasperl, 2015-07-03 07:49:55]

LGTM. Good stuff.

https://codereview.chromium.org/1209033003/diff/80001/lib/ffi/ffi.dart
File lib/ffi/ffi.dart (right):

https://codereview.chromium.org/1209033003/diff/80001/lib/ffi/ffi.dart#newcode14
lib/ffi/ffi.dart:14: int get value => _value;
I'd move the getter below the constructor. Do we need the getter at all in the
presence of the other more specific getters?

https://codereview.chromium.org/1209033003/diff/80001/lib/ffi/ffi.dart#newcode30
lib/ffi/ffi.dart:30: static final Foreign NULL = new Foreign._(0);
Where do you use this? Should it be a ForeignPointer instead?

https://codereview.chromium.org/1209033003/diff/80001/lib/ffi/ffi.dart#newcod...
lib/ffi/ffi.dart:155: /// ForeignLibrary used for looking up functions in the
linked in libraries.
This sentence no verb.

https://codereview.chromium.org/1209033003/diff/80001/lib/ffi/ffi.dart#newcod...
lib/ffi/ffi.dart:156: static ForeignLibrary standard = new
ForeignLibrary.fromName(null);
final? Maybe rename to main or builtin? The dlopen documentation calls this the
'main program'.

https://codereview.chromium.org/1209033003/diff/80001/lib/ffi/ffi.dart#newcod...
lib/ffi/ffi.dart:168: /// Provide a platform specific location for a library
relative to the
Provides

https://codereview.chromium.org/1209033003/diff/80001/lib/ffi/ffi.dart#newcod...
lib/ffi/ffi.dart:169: /// location of the fletch vm. Takes the name without lib
in front and
fletch -> Fletch

https://codereview.chromium.org/1209033003/diff/80001/lib/ffi/ffi.dart#newcod...
lib/ffi/ffi.dart:194: class ForeignPointer extends Foreign {
Move this up above the definition of ForeignLibrary?

https://codereview.chromium.org/1209033003/diff/80001/lib/ffi/ffi.dart#newcod...
lib/ffi/ffi.dart:214: ForeignMemory.allocatedFinalize(this._length) {
Not sure I like the name of this named constructor, but that isn't necessarily
your fault :) 

For users, I guess the most important thing isn't whether or not we use
finalization, but more if the memory is automatically freed.

https://codereview.chromium.org/1209033003/diff/80001/lib/io/system_posix.dart
File lib/io/system_posix.dart (right):

https://codereview.chromium.org/1209033003/diff/80001/lib/io/system_posix.dar...
lib/io/system_posix.dart:69: static final ForeignFunction _bind =
ForeignLibrary.standard.lookup("bind");
I'd prefer to break all the lines the same way to make the formatting more
consistent. So even for the cases where it fits on one line, I prefer splitting.
I'm weird. Thanks for asking.

https://codereview.chromium.org/1209033003/diff/80001/src/vm/ffi.cc
File src/vm/ffi.cc (right):

https://codereview.chromium.org/1209033003/diff/80001/src/vm/ffi.cc#newcode462
src/vm/ffi.cc:462: if (value->IsSmi()) {                                        
         \
I thin you missed my point here. I want to fold this together with the check for
wrong_argument_type to avoid repeating the checks. So:

  type* address = reinterpret_cast<type*>(AsForeignWord(arguments[0]));
  Object* value = arguments[1];
  if (value->IsSmi()) {
    *address = Smi::cast(value)->value();
  } else if (value->IsLargeInteger()) {
    *address = LargeInteger::cast(value)->value();
  } else {
    return Failure::wrong_argument_type();
  }
  return value;

https://codereview.chromium.org/1209033003/diff/80001/src/vm/ffi.h
File src/vm/ffi.h (right):

https://codereview.chromium.org/1209033003/diff/80001/src/vm/ffi.h#newcode33
src/vm/ffi.h:33: // In preparation of supporting windows we have a function for
this.
How about a courtesy newline before this comment?

https://codereview.chromium.org/1209033003/diff/80001/src/vm/object.h
File src/vm/object.h (right):

https://codereview.chromium.org/1209033003/diff/80001/src/vm/object.h#newcode179
src/vm/object.h:179: FOREIGNFUNCTION_MARKER  = 5,
FOREIGN_FUNCTION_MARKER
FOREIGN_MEMORY_MARKER

https://codereview.chromium.org/1209033003/diff/80001/src/vm/object.h#newcode...
src/vm/object.h:1203: 
Only one newline between functions.

https://codereview.chromium.org/1209033003/diff/80001/src/vm/process.cc
File src/vm/process.cc (right):

https://codereview.chromium.org/1209033003/diff/80001/src/vm/process.cc#newco...
src/vm/process.cc:884: Class* foreignmemory_class =
process->program()->foreignmemory_class();
foreign_memory_class

https://codereview.chromium.org/1209033003/diff/80001/src/vm/program.cc
File src/vm/program.cc (right):

https://codereview.chromium.org/1209033003/diff/80001/src/vm/program.cc#newco...
src/vm/program.cc:1420: InstanceFormat::instance_format(1,
Maybe reformat like this?

  InstanceFormat format = InstanceFormat::instance_format(
      1, InstanceFormat::FOREIGNFUNCTION_MARKER);

Repeated.

https://codereview.chromium.org/1209033003/diff/80001/src/vm/program.h
File src/vm/program.h (right):

https://codereview.chromium.org/1209033003/diff/80001/src/vm/program.h#newcode48
src/vm/program.h:48: V(Class, foreignfunction_class)                \
foreign_function_class
foreign_memory_class

https://codereview.chromium.org/1209033003/diff/80001/src/vm/session.cc
File src/vm/session.cc (right):

https://codereview.chromium.org/1209033003/diff/80001/src/vm/session.cc#newco...
src/vm/session.cc:774: printf("name: %d, %d", name, Names::kForeignFunction);
Debug printing?

https://codereview.chromium.org/1209033003/diff/80001/src/vm/session.cc#newco...
src/vm/session.cc:778: if (!(klass->instance_format().type() !=
InstanceFormat::INSTANCE_TYPE ||
Debug printing?

https://codereview.chromium.org/1209033003/diff/80001/tests/ffi/ffi_test.dart
File tests/ffi/ffi_test.dart (right):

https://codereview.chromium.org/1209033003/diff/80001/tests/ffi/ffi_test.dart...
tests/ffi/ffi_test.dart:305: 
Argh. Too many newlines :)

[ricow1, 2015-07-03 08:57:25]

https://codereview.chromium.org/1209033003/diff/80001/lib/ffi/ffi.dart
File lib/ffi/ffi.dart (right):

https://codereview.chromium.org/1209033003/diff/80001/lib/ffi/ffi.dart#newcode14
lib/ffi/ffi.dart:14: int get value => _value;
On 2015/07/03 07:49:54, kasperl wrote:
> I'd move the getter below the constructor. Do we need the getter at all in the
> presence of the other more specific getters?

removed

https://codereview.chromium.org/1209033003/diff/80001/lib/ffi/ffi.dart#newcode30
lib/ffi/ffi.dart:30: static final Foreign NULL = new Foreign._(0);
On 2015/07/03 07:49:54, kasperl wrote:
> Where do you use this? Should it be a ForeignPointer instead?

Done.

https://codereview.chromium.org/1209033003/diff/80001/lib/ffi/ffi.dart#newcod...
lib/ffi/ffi.dart:155: /// ForeignLibrary used for looking up functions in the
linked in libraries.
On 2015/07/03 07:49:53, kasperl wrote:
> This sentence no verb.

Done.

https://codereview.chromium.org/1209033003/diff/80001/lib/ffi/ffi.dart#newcod...
lib/ffi/ffi.dart:156: static ForeignLibrary standard = new
ForeignLibrary.fromName(null);
On 2015/07/03 07:49:53, kasperl wrote:
> final? Maybe rename to main or builtin? The dlopen documentation calls this
the
> 'main program'.

Renamed to main

https://codereview.chromium.org/1209033003/diff/80001/lib/ffi/ffi.dart#newcod...
lib/ffi/ffi.dart:168: /// Provide a platform specific location for a library
relative to the
On 2015/07/03 07:49:54, kasperl wrote:
> Provides

Done.

https://codereview.chromium.org/1209033003/diff/80001/lib/ffi/ffi.dart#newcod...
lib/ffi/ffi.dart:169: /// location of the fletch vm. Takes the name without lib
in front and
On 2015/07/03 07:49:54, kasperl wrote:
> fletch -> Fletch

Done.

https://codereview.chromium.org/1209033003/diff/80001/lib/ffi/ffi.dart#newcod...
lib/ffi/ffi.dart:194: class ForeignPointer extends Foreign {
On 2015/07/03 07:49:54, kasperl wrote:
> Move this up above the definition of ForeignLibrary?

Done.

https://codereview.chromium.org/1209033003/diff/80001/lib/ffi/ffi.dart#newcod...
lib/ffi/ffi.dart:214: ForeignMemory.allocatedFinalize(this._length) {
On 2015/07/03 07:49:53, kasperl wrote:
> Not sure I like the name of this named constructor, but that isn't necessarily
> your fault :) 
> 
> For users, I guess the most important thing isn't whether or not we use
> finalization, but more if the memory is automatically freed.

allocatedAutoFree?

https://codereview.chromium.org/1209033003/diff/80001/lib/io/system_posix.dart
File lib/io/system_posix.dart (right):

https://codereview.chromium.org/1209033003/diff/80001/lib/io/system_posix.dar...
lib/io/system_posix.dart:69: static final ForeignFunction _bind =
ForeignLibrary.standard.lookup("bind");
On 2015/07/03 07:49:54, kasperl wrote:
> I'd prefer to break all the lines the same way to make the formatting more
> consistent. So even for the cases where it fits on one line, I prefer
splitting.
> I'm weird. Thanks for asking.

Done.

https://codereview.chromium.org/1209033003/diff/80001/src/vm/ffi.cc
File src/vm/ffi.cc (right):

https://codereview.chromium.org/1209033003/diff/80001/src/vm/ffi.cc#newcode462
src/vm/ffi.cc:462: if (value->IsSmi()) {                                        
         \
On 2015/07/03 07:49:54, kasperl wrote:
> I thin you missed my point here. I want to fold this together with the check
for
> wrong_argument_type to avoid repeating the checks. So:
> 
>   type* address = reinterpret_cast<type*>(AsForeignWord(arguments[0]));
>   Object* value = arguments[1];
>   if (value->IsSmi()) {
>     *address = Smi::cast(value)->value();
>   } else if (value->IsLargeInteger()) {
>     *address = LargeInteger::cast(value)->value();
>   } else {
>     return Failure::wrong_argument_type();
>   }
>   return value;
Well, that is in the other cl, that already landed (with code exactly like what
you propose here). I did not update it on this branch, this will be merged and
have no changes here. The cl landed here:
https://github.com/dart-lang/fletch/commit/dd43d2cc4edab43b259ea9a4f053a5188b...
I did add a comment about that here :
https://codereview.chromium.org/1209033003/diff/60001/src/vm/ffi.cc , but that
disappeared from your view by me uploading one more patchset with some
indentation changes, sorry about that

https://codereview.chromium.org/1209033003/diff/80001/src/vm/ffi.h
File src/vm/ffi.h (right):

https://codereview.chromium.org/1209033003/diff/80001/src/vm/ffi.h#newcode33
src/vm/ffi.h:33: // In preparation of supporting windows we have a function for
this.
On 2015/07/03 07:49:54, kasperl wrote:
> How about a courtesy newline before this comment? 

Done.

https://codereview.chromium.org/1209033003/diff/80001/src/vm/object.h
File src/vm/object.h (right):

https://codereview.chromium.org/1209033003/diff/80001/src/vm/object.h#newcode179
src/vm/object.h:179: FOREIGNFUNCTION_MARKER  = 5,
On 2015/07/03 07:49:54, kasperl wrote:
> FOREIGN_FUNCTION_MARKER
> FOREIGN_MEMORY_MARKER

Done.

https://codereview.chromium.org/1209033003/diff/80001/src/vm/object.h#newcode...
src/vm/object.h:1203: 
On 2015/07/03 07:49:54, kasperl wrote:
> Only one newline between functions.

Done.

https://codereview.chromium.org/1209033003/diff/80001/src/vm/process.cc
File src/vm/process.cc (right):

https://codereview.chromium.org/1209033003/diff/80001/src/vm/process.cc#newco...
src/vm/process.cc:884: Class* foreignmemory_class =
process->program()->foreignmemory_class();
On 2015/07/03 07:49:54, kasperl wrote:
> foreign_memory_class

Done.

https://codereview.chromium.org/1209033003/diff/80001/src/vm/program.cc
File src/vm/program.cc (right):

https://codereview.chromium.org/1209033003/diff/80001/src/vm/program.cc#newco...
src/vm/program.cc:1420: InstanceFormat::instance_format(1,
On 2015/07/03 07:49:54, kasperl wrote:
> Maybe reformat like this?
> 
>   InstanceFormat format = InstanceFormat::instance_format(
>       1, InstanceFormat::FOREIGNFUNCTION_MARKER);
> 
> Repeated.
>   

Done.

https://codereview.chromium.org/1209033003/diff/80001/src/vm/program.h
File src/vm/program.h (right):

https://codereview.chromium.org/1209033003/diff/80001/src/vm/program.h#newcode48
src/vm/program.h:48: V(Class, foreignfunction_class)                \
On 2015/07/03 07:49:54, kasperl wrote:
> foreign_function_class
> foreign_memory_class

Done.

https://codereview.chromium.org/1209033003/diff/80001/src/vm/session.cc
File src/vm/session.cc (right):

https://codereview.chromium.org/1209033003/diff/80001/src/vm/session.cc#newco...
src/vm/session.cc:774: printf("name: %d, %d", name, Names::kForeignFunction);
On 2015/07/03 07:49:54, kasperl wrote:
> Debug printing?

Done.

https://codereview.chromium.org/1209033003/diff/80001/src/vm/session.cc#newco...
src/vm/session.cc:778: if (!(klass->instance_format().type() !=
InstanceFormat::INSTANCE_TYPE ||
On 2015/07/03 07:49:54, kasperl wrote:
> Debug printing?

Well in this case I would call it:
HowTheHellIsThisWireProtocolStuffWorkingAndWhereDoIChangeTheOtherEnd printing -
but removed since I guess the rest of you already know :-)

https://codereview.chromium.org/1209033003/diff/80001/tests/ffi/ffi_test.dart
File tests/ffi/ffi_test.dart (right):

https://codereview.chromium.org/1209033003/diff/80001/tests/ffi/ffi_test.dart...
tests/ffi/ffi_test.dart:305: 
On 2015/07/03 07:49:54, kasperl wrote:
> Argh. Too many newlines :)

Done.

[ricow1, 2015-07-03 11:04:45]

Committed patchset #6 (id:100001) manually as
97002260e4d6cfcb4d0e0d82d4ae3ade07defb96 (presubmit successful).