Skip to content

[vm/ffi] Change load() and store() to val and ref getter/setter pairs. #37361

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

Closed
sjindel-google opened this issue Jun 25, 2019 · 6 comments
Closed
Labels
area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends. library-ffi
Milestone

Comments

@sjindel-google
Copy link
Contributor

sjindel-google commented Jun 25, 2019

In the new structs API (#37229) we will remove load() and store() in favor of a val getter/setter pair for non-struct pointers and a ref getter for pointers to structs:

@pragma("vm:entry-point")
class Pointer<T extends NativeType> extends NativeType {
  const Pointer();

  external factory Pointer.allocate({int count: 1});

  external factory Pointer.fromAddress(int ptr);

  external set val(@DartRepresentationOf("T") Object value);

  external dynamic get val;

  external T get ref;

  external int get address;

  external Pointer<U> cast<U extends NativeType>();
}

This task includes implementation of the decisions make in #35782 and #35756.

@sjindel-google sjindel-google added area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends. library-ffi labels Jun 25, 2019
@sjindel-google sjindel-google self-assigned this Jun 25, 2019
@dcharkes dcharkes removed their assignment Jul 8, 2019
@dcharkes dcharkes added this to the D26 Release milestone Sep 30, 2019
@dcharkes
Copy link
Contributor

This will be addressed in the same CL that changes the API to extension methods (#37773).

@lrhn
Copy link
Member

lrhn commented Oct 8, 2019

About val and ref:
Definitely use value instead of val. In general the style guide says to avoid abbreviations unless they are the common way to refer to the thing. The val identifier is not usually used for reading values, more for declaring variables (var/val). It's also still short.

The ref is more tricky, both because ref is a commonly used abbreviation and because the alternatives are longer and more cumbersome.
The idiomatic declaration would be asReference(). It returns a new view of (parts of) the original object. Perhaps toReference(), but it doesn't feel like a conversion to me, more like an adaption.

That is what I would recommend. If you think it is too long, and that it will be used a lot, then going for ref might not be that bad.

@dcharkes
Copy link
Contributor

dcharkes commented Oct 8, 2019

Thanks @lrhn.

You can see the impact of .ref or .asReference() in this CL. If we go for asReference() instead of .ref the following code would be the most cumbersome in our own codebase:

  Pointer<Coordinate> c1 = Pointer.allocate(count: 3);
  Pointer<Coordinate> c2 = c1.elementAt(1);
  Pointer<Coordinate> c3 = c1.elementAt(2);
  c1.ref.x = 10.0;
  c1.ref.y = 10.0;
  c1.ref.next = c3;
  c2.ref.x = 20.0;
  c2.ref.y = 20.0;
  c2.ref.next = c1;
  c3.ref.x = 30.0;
  c3.ref.y = 30.0;
  c3.ref.next = c2;

But we could argue that that should be rewritten as:

  // ...
  c3.ref
     ..x = 30.0
     ..y = 30.0
     ..next = c2;

Maybe going for the idiomatic API .asReference() would not be as cumbersome as expected. @mkustermann @sjindel-google, does any of you expect user code to have significantly more uses of .asReference()/.ref than our own test code?

@sjindel-google
Copy link
Contributor Author

The C equivalent of x.ref is simply *x, which is why I would favor the .ref syntax. I do think user code will use this a lot, especially if it's passing structs to FFI functions, because there they're always represented as pointers, and they need to do .ref to actually use the structs.

@mkustermann
Copy link
Member

I vote for .ref over .asReference()

@dcharkes
Copy link
Contributor

dcharkes commented Oct 8, 2019

Implemented in 597cd06.

@dcharkes dcharkes closed this as completed Oct 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends. library-ffi
Projects
None yet
Development

No branches or pull requests

4 participants