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

Making type IDs accessible from Rust code #2

Merged
merged 5 commits into from Dec 14, 2014
Merged

Making type IDs accessible from Rust code #2

merged 5 commits into from Dec 14, 2014

Conversation

ghost
Copy link

@ghost ghost commented Dec 13, 2014

These patches should allow the use of Capn'Proto type ids from Rust code.

For a Capn'Proto struct, it declares a const TYPE_ID: u64 in the module defining the struct, and the Reader and Builder both implement the HasTypeId trait defining a type_id(_ : Option<Self>) function (no better way to call it before UFCS).

For an enum, it only makes the enum implement the HasTypeId trait.

For an interface, it declare a const INTERFACE_ID: u64 in the module defining the interface, and the Client implements the HasTypeId trait

This pull request requires the HasTypeId trait in the runtime library.

@@ -1286,6 +1320,7 @@ fn generate_node(node_map : &collections::hash_map::HashMap<u64, schema_capnp::n
Line("use capnp::capability::{ClientHook, FromClientHook, FromServer, Request, ServerHook};".to_string()));
mod_interior.push(Line("use capnp::capability;".to_string()));
mod_interior.push(BlankLine);
mod_interior.push(Line(format!("pub const INTERFACE_ID: u64 = {:#x};", node_id)));
Copy link
Member

Choose a reason for hiding this comment

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

I feel like this ought to be called TYPE_ID as well. That would be more consistent with the C++ version (see the definition of CAPNP_DECLARE_INTERFACE_HEADER in generated_header_support.h). Do you have a reason to prefer INTERFACE_ID?

Copy link
Author

Choose a reason for hiding this comment

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

Well I thought INTERFACE_ID would be a little bit more explicit, but I'm not really attached to it ...

@dwrensha
Copy link
Member

Looks good! I just have a few nitpicks, as discussed in my other comments.

@dwrensha dwrensha merged commit e55ed99 into capnproto:master Dec 14, 2014
@dwrensha
Copy link
Member

I went ahead and merged your branch that had the private inner module. Thanks!

@ghost
Copy link
Author

ghost commented Dec 14, 2014

Oops, Sorry! Looks like I took too long to do the last fixes!
vaelden/capnpc-rust@2eed1ce81ac7696f0c5b639aea9aba19e5acc602 should make the necessary changes to remove unused imports and make TYPE_ID accessible for groups.

@dwrensha
Copy link
Member

No worries! I've just merged that commit in too.

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

1 participant