Skip to content
This repository has been archived by the owner on Sep 21, 2024. It is now read-only.

feat: Initial example of C integration #242

Merged
merged 1 commit into from
Mar 8, 2023
Merged

feat: Initial example of C integration #242

merged 1 commit into from
Mar 8, 2023

Conversation

jsantell
Copy link
Contributor

Need to figure out some text encoding stuff and clean ups, but this C snippet links and runs against libnoosphere

@jsantell jsantell force-pushed the c branch 3 times, most recently from 4de517c to b1dc317 Compare February 23, 2023 00:57
@jsantell
Copy link
Contributor Author

Now working. Some areas of improvements could be tying it into the larger build system:

  • maybe lives in the noosphere crate?
  • make is naive here as it's not rebuilding rust if it changes.
  • Leak detection

Initial reactions to working with the raw FFI:

  • Mostly copied the Swift tests, s/nil/NULL, and added semi-colons and it was smooth sailing other than remembering C trivia
  • The generic wrapper types (slice_ref_uint8_t, slice_boxed_char_ptr_t) tripped me up, learning how safer_ffi maps Rust to C, initially criticizing the non-pointer args, but going back on that now. A small win here could be possibly renaming the generated types: slice_boxed_char_ptr_t -> ns_string_array_t, slice_boxed_uint8_t -> ns_bytes -- matching their corresponding free functions. But this challenge extends to all Rust-owned primitive types. Need to think more about this, wonder how other libraries handle this.
  • The similarly named slice_boxed_uint8_t and slice_ref_uint8_t caused some confusion as well, with a slight difference in pointer types (ref being the const version) -- the "ref" type is for (from Rust's perspective) unowned data, used in args, and the "boxed" type is for return values. To simplify these intermediate types, maybe we can just use boxed for return values, and use two args (array, length) where needed (only ns_sphere_fs_write uses the ref type currently).
  • Idiomatically, would love NsSphereFs_t -> ns_sphere_fs_t! Need to investigate feasibility of doing this in safer_ffi (meta issue: Customizable naming templates (e.g., namespace prefixing) getditto/safer_ffi#93)
  • I had to link libmath (-lm clang/gcc arg) to get Noosphere linked, but that seems to be the only dependency

@jsantell jsantell requested a review from cdata February 23, 2023 01:23
Copy link
Collaborator

@cdata cdata left a comment

Choose a reason for hiding this comment

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

I love that we can add C to our retinue of supported languages. Some general feedback:

Can we put the example in a sub-folder? e.g., $ROOT/c/example/main.c

And, can we add a README.md at $ROOT/c/README.md? It doesn't have to be very long, but otherwise model it roughly after the other READMEs in our language folders.

Super cool :D

@cdata
Copy link
Collaborator

cdata commented Feb 24, 2023

Oh, also: please file an issue to get a test workflow in place for CI. It would be nice to be made aware of conspicuous support regressions.

@jsantell jsantell force-pushed the c branch 2 times, most recently from bc31cd7 to 11a26c5 Compare February 24, 2023 20:04
@jsantell jsantell requested a review from cdata February 24, 2023 20:06
@jsantell
Copy link
Contributor Author

  • Added HeaderTransformer to transform generated header to C idiomatic types: NsSphereFs_t to ns_sphere_fs_t. Makes it possible to programmatically tag classes with correct Doxygen tags.
  • Added explicit type casts between uint8_t* and char* to satiate clang (and actually try out both clang and gcc)
  • Moved files around to make c/ a root language directory like others
  • Filed Setup CI tests for C bindings #245

* Add base example and build script in C
* Transform generated C headers from safer_ffi with idiomatic type
  names.
Copy link
Collaborator

@cdata cdata left a comment

Choose a reason for hiding this comment

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

The transformer is very interesting 😳 we'll have to keep an eye on it to make sure we don't end up with any funky transformations.

@cdata cdata merged commit 57beb24 into main Mar 8, 2023
@cdata cdata deleted the c branch March 8, 2023 16:16
@github-actions github-actions bot mentioned this pull request Mar 8, 2023
This was referenced Jul 1, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants