-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Conversation
this is requested here dart-lang#55971
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). |
@codesculpture please see, and respond to, review feedback in https://dart-review.googlesource.com/c/sdk/ /378221 |
https://dart-review.googlesource.com/c/sdk/ /378221 has been updated with the latest commits from this pull request. |
`.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.
https://dart-review.googlesource.com/c/sdk/ /378221 has been updated with the latest commits from this pull request. |
1 similar comment
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
https://dart-review.googlesource.com/c/sdk/ /378221 has been updated with the latest commits from this pull request. |
1 similar comment
https://dart-review.googlesource.com/c/sdk/ /378221 has been updated with the latest commits from this pull request. |
https://dart-review.googlesource.com/c/sdk/ /378221 has been updated with the latest commits from this pull request. |
1 similar comment
https://dart-review.googlesource.com/c/sdk/ /378221 has been updated with the latest commits from this pull request. |
such as `buffer[0].address.cast()`, so that adding a new test function in `ffi_test_functions.cc` named `SumTwoPointers` This been suggested by Darco Charkes.
https://dart-review.googlesource.com/c/sdk/ /378221 has been updated with the latest commits from this pull request. |
1 similar comment
https://dart-review.googlesource.com/c/sdk/ /378221 has been updated with the latest commits from this pull request. |
https://dart-review.googlesource.com/c/sdk/ /378221 has been updated with the latest commits from this pull request. |
1 similar comment
https://dart-review.googlesource.com/c/sdk/ /378221 has been updated with the latest commits from this pull request. |
https://dart-review.googlesource.com/c/sdk/ /378221 has been updated with the latest commits from this pull request. |
(I have already reviewed. Currently waiting for addressing of comments from analyzer team.) |
I will address the comment asap @dcharkes |
…as that should be enough and thats used across the file and added comments in useSites.dart
…nges in mock_sdk to support unit tests
https://dart-review.googlesource.com/c/sdk/ /378221 has been updated with the latest commits from this pull request. |
1 similar comment
https://dart-review.googlesource.com/c/sdk/ /378221 has been updated with the latest commits from this pull request. |
https://dart-review.googlesource.com/c/sdk/ /378221 has been updated with the latest commits from this pull request. |
1 similar comment
https://dart-review.googlesource.com/c/sdk/ /378221 has been updated with the latest commits from this pull request. |
https://dart-review.googlesource.com/c/sdk/ /378221 has been updated with the latest commits from this pull request. |
1 similar comment
https://dart-review.googlesource.com/c/sdk/ /378221 has been updated with the latest commits from this pull request. |
https://dart-review.googlesource.com/c/sdk/ /378221 has been updated with the latest commits from this pull request. |
1 similar comment
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
https://dart-review.googlesource.com/c/sdk/ /378221 has been updated with the latest commits from this pull request. |
1 similar comment
https://dart-review.googlesource.com/c/sdk/ /378221 has been updated with the latest commits from this pull request. |
https://dart-review.googlesource.com/c/sdk/ /378221 has been updated with the latest commits from this pull request. |
1 similar comment
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)
https://dart-review.googlesource.com/c/sdk/ /378221 has been updated with the latest commits from this pull request. |
1 similar comment
https://dart-review.googlesource.com/c/sdk/ /378221 has been updated with the latest commits from this pull request. |
https://dart-review.googlesource.com/c/sdk/ /378221 has been updated with the latest commits from this pull request. |
1 similar comment
https://dart-review.googlesource.com/c/sdk/ /378221 has been updated with the latest commits from this pull request. |
No description provided.