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

Refactor Android property key CStr construction to add tests #69

Merged
merged 2 commits into from Oct 8, 2022

Conversation

lopopolo
Copy link
Collaborator

@lopopolo lopopolo commented Sep 30, 2022

Add a new FFI utils module that is always tested on all platforms.

Extract the C string bytes to a const and getter function, add more docs, add tests that the const can be safely turned into a CStr.

@lopopolo lopopolo added the Unsafe-Proposed `unsafe` code is added or changed label Sep 30, 2022
Copy link
Member

@astraw astraw left a comment

Choose a reason for hiding this comment

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

Looks good. Thanks.

@Kijewski
Copy link
Collaborator

Kijewski commented Sep 30, 2022

I updated the CI to make sure the test is executed, because I don't think anyone of us will want to try every commit on our actual phones. :)

src/lib.rs Outdated Show resolved Hide resolved
.github/workflows/rust.yml Outdated Show resolved Hide resolved
Add a new FFI utils module that is always tested on all platforms.

Extract the C string bytes to a const and getter function, add more
docs, add tests that the const can be safely turned into a `CStr`.
@lopopolo lopopolo changed the title Add test to assert Android CStr correctness Refactor Android property key CStr construction to add tests Sep 30, 2022
@lopopolo
Copy link
Collaborator Author

lopopolo commented Sep 30, 2022

@astraw @Kijewski I took another stab at this to reduce some of the complexity in CI and add more tests + additional safety checks in CI and debug mode.

src/ffi_utils.rs Outdated Show resolved Hide resolved
@Kijewski Kijewski added the Tier-2 Rust Tier-2 platform label Oct 1, 2022
@Kijewski
Copy link
Collaborator

Kijewski commented Oct 1, 2022

I added Tier- to Tier-3 labels to match https://doc.rust-lang.org/nightly/rustc/platform-support.html. E.g Android is a Tier 2 platform.

@lopopolo
Copy link
Collaborator Author

lopopolo commented Oct 1, 2022

Alright we have two approvals, I've addressed all of the comments, and the build is green. Since this has the Unsafe-Proposed label, I'll merge this on October 7.

@lopopolo
Copy link
Collaborator Author

lopopolo commented Oct 1, 2022

I added Tier- to Tier-3 labels to match https://doc.rust-lang.org/nightly/rustc/platform-support.html. E.g Android is a Tier 2 platform.

Thanks for this! I went through the full history of PRs and issues and added these tags the best I could.

@astraw
Copy link
Member

astraw commented Oct 1, 2022

Great, thanks @lopopolo .

This is now ready to merge. Given its Unsafe-Proposed status, the 7 day timer starts now. So we should merge this on 8 October.

@astraw
Copy link
Member

astraw commented Oct 2, 2022

(@lopopolo somehow I missed your earlier message about merging on 7 October. Sorry about that!)

@astraw astraw merged commit 1f38743 into main Oct 8, 2022
lopopolo added a commit that referenced this pull request Oct 14, 2022
@astraw astraw deleted the lopopolo/android-cstring-test branch October 16, 2022 06:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Tier-2 Rust Tier-2 platform Unsafe-Proposed `unsafe` code is added or changed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants