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

#14 Clean up a bunch of repeated patterns and use result/option where useful. Convert structs to classes to add deinit to clear memory #16

Merged
merged 11 commits into from
Apr 29, 2020

Conversation

ernieturner
Copy link
Contributor

No description provided.

result.initialize(from: baseAddress, count: count)
}
return CRustStrView(data: result, len: UInt(str.utf8.count))
static func intFromBool(_ b: Bool) -> Int8 {
Copy link
Member

Choose a reason for hiding this comment

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

For consistency I'd name this boolToInt.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

/**
* Return true if the provided Int8 represents a true value
*/
static func isTrue(_ int: Int8) -> Bool {
Copy link
Member

Choose a reason for hiding this comment

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

I would name this intToBool for consistency.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

* Convert the provided Array of IronOxide bytes into Swift UInt8 bytes
*/
static func toBytes(_ bytes: CRustVeci8) -> [UInt8] {
Array(UnsafeBufferPointer(start: bytes.data, count: Int(bytes.len))).map(UInt8.init)
Copy link
Member

Choose a reason for hiding this comment

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

We should comment here about expecting those bytes to be copied into swift managed memory.

Also, does anything free the CRustVeci8? I guess whenever the structure it was in gets freed we're just counting on it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a comment. Following up about freeing the Rust bytes in Slack.

Sources/IronOxide/Util.swift Show resolved Hide resolved
Sources/IronOxide/Util.swift Show resolved Hide resolved
…e method names and free Rust memory where possible
Copy link
Member

@coltfred coltfred left a comment

Choose a reason for hiding this comment

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

Couple cleanups in the README, but overall LGTM.

+ Now you can compile this project via `swift build -Xlinker -L{path_to_your_ironoxide_target_ios_release_directory}`. If successful this should generate a `.build` directory which is similar to the `target` directory generated in Rust projects.
+ This repo should be checked out as a sibling of the [IronOxide Java repo](https://github.com/IronCoreLabs/ironoxide-java).
+ Build the C/C++ bindings within your `ironoxide-java` checkout
+ [Mac Only?] Go into the `cpp` directory and modify the `Cargo.toml` file to build a `staticlib` instead of a `dylib`
Copy link
Member

Choose a reason for hiding this comment

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

I did this change on linux too, but I haven't tried without it.


## Adding Library to iOS Project in XCode

XCode dependencies are added via URLs, usually pointing directly to a GitHub repository. You can also use the `file://` protocol to have it point to a local library on disk.

+ Compile the `ironoxide-java` binary for the `x86_64-apple-ios` target. From the `ironoxide-java/cpp` directry run `cargo build --release --target x86_64-apple-ios`
Copy link
Member

Choose a reason for hiding this comment

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

This isn't right for linux. I just didn't put --target on it at all.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is in the section for how to add this library to an iOS project in XCode where this is necessary. The steps above about getting this library setup don't include this.

@ernieturner ernieturner merged commit 1a511fa into master Apr 29, 2020
@ernieturner ernieturner deleted the cleanup branch April 29, 2020 19:23
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.

None yet

4 participants