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

Fixed a bug with zero-copy cargo feature #41

Merged
merged 9 commits into from
May 29, 2023
Merged

Fixed a bug with zero-copy cargo feature #41

merged 9 commits into from
May 29, 2023

Conversation

temeddix
Copy link
Contributor

@temeddix temeddix commented May 27, 2023

I am sorry to tell that I left out one of the most important changes when I was making the previous #39 PR.

self should be passed to ManuallyDrop instead of self.0, because this one doesn't have a ZeroCopyBuffer wrapper. I apologize for the mistake.

src/into_dart.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@fzyzcjy fzyzcjy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(see above)

@temeddix
Copy link
Contributor Author

temeddix commented May 28, 2023

I made a vec_to_dart_native_external_typed_data function to avoid code duplication. Also, I added CI tests with cargo features enabled, which uses cargo fmt and clippy, that also tells you about an error.

@shekohex
Copy link
Owner

Could we take a moment to think about how would be able to test something like that in our Mocked DartVM vm.rs?

@fzyzcjy did you figure out a way to do End to End testing with Memory checks using real Dart VM And Rust?

@fzyzcjy
Copy link
Contributor

fzyzcjy commented May 29, 2023

@fzyzcjy did you figure out a way to do End to End testing with Memory checks using real Dart VM And Rust?

Check the valgrind section in my ci.yaml. However, I just checked memory leaks, and do not check invalid memory accesses, because Dart seems to violate so many valgrind operations...

Related: dart-lang/sdk#47346

@shekohex
Copy link
Owner

@fzyzcjy did you figure out a way to do End to End testing with Memory checks using real Dart VM And Rust?

Check the valgrind section in my ci.yaml. However, I just checked memory leaks, and do not check invalid memory accesses, because Dart seems to violate so many valgrind operations...

Related: dart-lang/sdk#47346

Fair enough. @temeddix are you open to make a PR that introduce these kind of checks into our code and then we merge it here into your PR? I want to make sure that we do not introduce any other memory bugs because of a new feature we added.

@temeddix
Copy link
Contributor Author

temeddix commented May 29, 2023

I'm not familiar with valgrind tests, but I'll try to apply those in a few days :) I'll refer to flutter_rust_bridge repo.

@temeddix
Copy link
Contributor Author

temeddix commented May 29, 2023

I dug into the valgrind test code of flutter_rust_bridge for some time, and unfortunately, it is too complicated for me to make that work here.

In flutter_rust_bridge, Github CI calls a justfile command, and that justfile command calls many other justfile commands, some of which call the .sh bash commands in the Dart folder, which calls a Python code, which executes valgrind. This logic is not simple enough for me to fully understand properly.

Apart from this 'deep call stack', there's no Dart example like flutter_rust_bridge in this repo. There are only Rust files here, which means many Dart codes for testing the library's features should be added. I'm not sure if I can understand all the features of this library and organize it into a testing Dart file. This complexity deserves a whole new PR :|

In my opinion, we could focus on fixing the bug and adding CI for Rust linting error in this PR.

@shekohex
Copy link
Owner

Fair enough. Okay I guess I will go an merge this PR.

@shekohex shekohex merged commit d74d25e into shekohex:master May 29, 2023
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