Skip to content

Borsch traits using bindgen #196

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

Merged
merged 13 commits into from
Jul 22, 2022
Merged

Borsch traits using bindgen #196

merged 13 commits into from
Jul 22, 2022

Conversation

majabbour
Copy link
Contributor

We need to derive Borsh traits to C bindings, so I changed build.rs to include a map from type names to traits that we can use to add traits to types in the C bindings. I included the output file in c_oracle_header.rs below imports needed to define the added traits.

@majabbour majabbour force-pushed the borsch-traits-using-bindgen branch from 1fcd5ae to 126d497 Compare July 21, 2022 15:38
@majabbour majabbour requested review from tompntn and jayantk July 21, 2022 15:50
Copy link
Contributor

@guibescos guibescos left a comment

Choose a reason for hiding this comment

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

nitpick : Can you replace this syntax use bindgen; later in the code bindgen::callbacks::ParseCallbacks by use bindgen::callbacks::ParseCallbacks and later in the code simply ParseCallbacks. This seems more consistent with the syntax we normally use.

])};

//generate and write bindings
let bindings = bindgen::Builder::default().header("./src/bindings.h").parse_callbacks(Box::new(parser)).generate().expect("Unable to generate bindings");
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpic: would be good to run rustfmt on all Rust code in this repo as part of the CI/CD to get consistent formatting.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh sorry I have not been doing that for the code I have been pushing, I should probably make a different PR with just cargo fmt after merging this/ other outstanding rust PRs. Otherwise git diff might be confusing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a all to fmt in to the Builder in build.rs. Did not seem to change how bindings.rs lookd much, but I do not think it's a big problem since you should never have to read it (because it just redefines stuff in oracle.h).

I will start calling cargo fmt on every commit once we make that fmt PR

@majabbour majabbour force-pushed the borsch-traits-using-bindgen branch 2 times, most recently from 22d2ce1 to 2a8e45e Compare July 21, 2022 19:12

let borsh_derives: Vec<String> = vec!["BorshSerialize".to_string(), "BorshDeserialize".to_string()];

//make a parser and to it type, traits pairs
Copy link
Contributor

Choose a reason for hiding this comment

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

i think people usually put a space after the // and before the text. Actually I'm kind of surprised that rustfmt doesn't fix this for you.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, sorry noob Rust developer here. Forgot to run cargo fmt. Once this and the other PR are approved, I will do it in a seperate PR that is just fmt. (and then run it after every commit).

If I run it now the diffs will probably be confusing for reviewers.

Copy link
Contributor

@jayantk jayantk 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 to me -- definitely better to rely on rust to add these traits. I'll let @guibescos give you the final approval though.

mod c_oracle_header;

//Below is a high lever description of the rust/c setup.
Copy link
Contributor

Choose a reason for hiding this comment

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

🙇 for writing documentation explaining how this works.

@majabbour majabbour force-pushed the borsch-traits-using-bindgen branch from 4697f3b to 5871ab8 Compare July 21, 2022 21:48
@majabbour majabbour requested a review from guibescos July 21, 2022 22:36
@guibescos guibescos merged commit d522ff4 into main Jul 22, 2022
@guibescos guibescos deleted the borsch-traits-using-bindgen branch July 22, 2022 17:04
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.

4 participants