Skip to content
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

[vm/ffi] Supporting .address.cast() expression in ffi leaf calls #56357

Closed
wants to merge 31 commits into from

Conversation

codesculpture
Copy link
Contributor

No description provided.

Copy link

Thank you for your contribution! This project uses Gerrit for code reviews. Your pull request has automatically been converted into a code review at:

https://dart-review.googlesource.com/c/sdk/ /378221

Please wait for a developer to review your code review at the above link; you can speed up the review if you sign into Gerrit and manually add a reviewer that has recently worked on the relevant code. See CONTRIBUTING.md to learn how to upload changes to Gerrit directly.

Additional commits pushed to this PR will update both the PR and the corresponding Gerrit CL. After the review is complete on the CL, your reviewer will merge the CL (automatically closing this PR).

@mit-mit mit-mit marked this pull request as ready for review August 2, 2024 10:12
@mit-mit
Copy link
Member

mit-mit commented Aug 2, 2024

@codesculpture please see, and respond to, review feedback in https://dart-review.googlesource.com/c/sdk/ /378221

Copy link

https://dart-review.googlesource.com/c/sdk/ /378221 has been updated with the latest commits from this pull request.

@codesculpture
Copy link
Contributor Author

Sorry @mit-mit , i dint noticed there. Its just a WIP and been discussed with @dcharkes.
Will look on it, Thanks

`.address.cast()` expression

Been suggested by Daco Harkes
Since we allowed `.address.cast()` and handled in ffi dea013d

`.cast()` made the `node` one level nested down in the hierarchy,
so we need to traverse one level up to reach the actual ffi Invocation

For ex:
    myNative(a.address) -> we can reach from `a.address` to `myNative` by traversing two levels
    where (a.address) -> ArgumentList then its parent would be myNative(a.address) -> MethodInvocation

    myNative(a.address.cast()) -> here we need to traverse from `a.address` to `a.address.cast()` this would be
    extra traversal required and then we can reach the ffi Invocation as we already did.
Copy link

https://dart-review.googlesource.com/c/sdk/ /378221 has been updated with the latest commits from this pull request.

1 similar comment
Copy link

https://dart-review.googlesource.com/c/sdk/ /378221 has been updated with the latest commits from this pull request.

the class `Pointer` from `dart:ffi` using isPointer
Copy link

https://dart-review.googlesource.com/c/sdk/ /378221 has been updated with the latest commits from this pull request.

1 similar comment
Copy link

https://dart-review.googlesource.com/c/sdk/ /378221 has been updated with the latest commits from this pull request.

Copy link

https://dart-review.googlesource.com/c/sdk/ /378221 has been updated with the latest commits from this pull request.

1 similar comment
Copy link

https://dart-review.googlesource.com/c/sdk/ /378221 has been updated with the latest commits from this pull request.

Copy link

https://dart-review.googlesource.com/c/sdk/ /378221 has been updated with the latest commits from this pull request.

1 similar comment
Copy link

https://dart-review.googlesource.com/c/sdk/ /378221 has been updated with the latest commits from this pull request.

Copy link

https://dart-review.googlesource.com/c/sdk/ /378221 has been updated with the latest commits from this pull request.

1 similar comment
Copy link

https://dart-review.googlesource.com/c/sdk/ /378221 has been updated with the latest commits from this pull request.

Copy link

https://dart-review.googlesource.com/c/sdk/ /378221 has been updated with the latest commits from this pull request.

@lrhn lrhn requested a review from dcharkes August 16, 2024 11:17
@dcharkes
Copy link
Contributor

(I have already reviewed. Currently waiting for addressing of comments from analyzer team.)

@codesculpture
Copy link
Contributor Author

I will address the comment asap @dcharkes
Just got some personal work to be done.

…as that should be enough and thats used across the file and added comments in useSites.dart
Copy link

https://dart-review.googlesource.com/c/sdk/ /378221 has been updated with the latest commits from this pull request.

1 similar comment
Copy link

https://dart-review.googlesource.com/c/sdk/ /378221 has been updated with the latest commits from this pull request.

Copy link

https://dart-review.googlesource.com/c/sdk/ /378221 has been updated with the latest commits from this pull request.

1 similar comment
Copy link

https://dart-review.googlesource.com/c/sdk/ /378221 has been updated with the latest commits from this pull request.

Copy link

https://dart-review.googlesource.com/c/sdk/ /378221 has been updated with the latest commits from this pull request.

1 similar comment
Copy link

https://dart-review.googlesource.com/c/sdk/ /378221 has been updated with the latest commits from this pull request.

Copy link

https://dart-review.googlesource.com/c/sdk/ /378221 has been updated with the latest commits from this pull request.

1 similar comment
Copy link

https://dart-review.googlesource.com/c/sdk/ /378221 has been updated with the latest commits from this pull request.

… cast as now it was included in the mock_sdk
Copy link

https://dart-review.googlesource.com/c/sdk/ /378221 has been updated with the latest commits from this pull request.

1 similar comment
Copy link

https://dart-review.googlesource.com/c/sdk/ /378221 has been updated with the latest commits from this pull request.

Copy link

https://dart-review.googlesource.com/c/sdk/ /378221 has been updated with the latest commits from this pull request.

1 similar comment
Copy link

https://dart-review.googlesource.com/c/sdk/ /378221 has been updated with the latest commits from this pull request.

…s) doesn't necessary (suggested by daco charkes)
Copy link

https://dart-review.googlesource.com/c/sdk/ /378221 has been updated with the latest commits from this pull request.

1 similar comment
Copy link

https://dart-review.googlesource.com/c/sdk/ /378221 has been updated with the latest commits from this pull request.

Copy link

https://dart-review.googlesource.com/c/sdk/ /378221 has been updated with the latest commits from this pull request.

1 similar comment
Copy link

https://dart-review.googlesource.com/c/sdk/ /378221 has been updated with the latest commits from this pull request.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants