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

Add pull/get_page API #3

Merged
merged 5 commits into from
Oct 21, 2024
Merged

Add pull/get_page API #3

merged 5 commits into from
Oct 21, 2024

Conversation

Saafo
Copy link
Contributor

@Saafo Saafo commented Aug 7, 2024

There are few things I'm not sure yet:

  • I'm not sure whether we should use Vec::from_raw_parts for higher performance (I encountered with panic pointer being freed was not allocated, seems they don't use the same allocator?)
  • Not sure std::ptr::drop_in_place would free the memory allocated by C, should we add a dependency libc to use libc::free?
  • Whether we can add an examples/ or tests/ besides the readme

Hi @d-k-bo, do you have time to review this PR? I wish we can have a release when this PR is merged.

@Saafo Saafo marked this pull request as ready for review August 8, 2024 03:15
@Saafo Saafo marked this pull request as draft August 8, 2024 03:33
Change-Id: I2fbd448abd39cc123f483bc19963c1f7c75b8122
@Saafo
Copy link
Contributor Author

Saafo commented Aug 8, 2024

The two issue about freeing pointer is solved: the pointer returned by ope_encoder_get_page doesn't need to be freed since it's a slice.

@Saafo Saafo marked this pull request as ready for review August 8, 2024 07:16
@d-k-bo
Copy link
Owner

d-k-bo commented Aug 16, 2024

Sorry for my late response. I was on vacation and now I am pretty busy preparing for an upcoming exam. It might take a while until I can look into this.

src/encoder.rs Outdated Show resolved Hide resolved
src/encoder.rs Outdated Show resolved Hide resolved
@Saafo
Copy link
Contributor Author

Saafo commented Oct 14, 2024

Good suggestions! Fixed and request review again ❤️

@Saafo Saafo requested a review from d-k-bo October 14, 2024 05:24
@d-k-bo
Copy link
Owner

d-k-bo commented Oct 16, 2024

Whether we can add an examples/ or tests/ besides the readme

While it would be great to have proper unit tests (for the entire library), an example that shows how this API can be used would be a really good idea (either as an example in examples/ or as part of the docs for Encoder::create_pull).

@Saafo
Copy link
Contributor Author

Saafo commented Oct 18, 2024

@d-k-bo I'm afraid that my example/test code is not ready yet. Can we merge this pr first?

examples/pull_api.rs Outdated Show resolved Hide resolved
@d-k-bo d-k-bo merged commit c36abce into d-k-bo:main Oct 21, 2024
4 checks passed
@d-k-bo
Copy link
Owner

d-k-bo commented Oct 21, 2024

Thanks for your contribution!

@d-k-bo
Copy link
Owner

d-k-bo commented Oct 21, 2024

It is now available in version 0.2.2 on crates.io.

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.

2 participants