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

Non-panicking capnp::struct_list::Reader::get #251

Closed
marmeladema opened this issue Jan 7, 2022 · 2 comments · Fixed by #285
Closed

Non-panicking capnp::struct_list::Reader::get #251

marmeladema opened this issue Jan 7, 2022 · 2 comments · Fixed by #285

Comments

@marmeladema
Copy link
Contributor

Hello!

Currently capnp::struct_list::Reader::get can panic if the index is out of bounds because of an assertion at https://docs.capnproto-rust.org/src/capnp/struct_list.rs.html#84

This is rather unexpected as the documentation doesn't say so and the usual convention for get method of collections is to return an Option<T> for in invalid index/key/etc.

Would it make sense to (1) change the prototype of capnp::struct_list::Reader::get to return an option and maybe (2) implement the Index trait with the old panicking behavior as done for slices in std?

I am willing to provide the PR, I am just unsure of what non backward compatible change are deemed acceptable.

@dwrensha
Copy link
Member

dwrensha commented Jan 15, 2022

We can't use std::ops::Index, because its index() method returns a reference, which we generally can't do because we may need to swap bytes of the value to account for endianness (in the case of primitive_list).

We have an IndexMove trait for this purpose:

pub trait IndexMove<I, T> {
fn index_move(&self, index: I) -> T;
}

There have been proposals for adding something like it upstream: rust-lang/rfcs#997

@marmeladema
Copy link
Contributor Author

We can't use std::ops::Index, because its index() method returns a reference, which we generally can't do because we may need to swap bytes of the value to account for endianness (in the case of primitive_list).

Right, that's unfortunate. That being said, I am more interested in having a non-panicking version of get methods.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants