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

Convert (consume) Builder into TypedReader #84

Merged
merged 2 commits into from
Jan 31, 2018
Merged

Convert (consume) Builder into TypedReader #84

merged 2 commits into from
Jan 31, 2018

Conversation

bspeice
Copy link
Contributor

@bspeice bspeice commented Jan 25, 2018

Accidentally PR'd into my forked master, not original repo master.
Addresses #83

@bspeice
Copy link
Contributor Author

bspeice commented Jan 25, 2018

I don't know how we should handle the ReaderOptions - I.e. is it safe to just use the default, or should I expose the ability to pass in an external ReaderOptions?

@dwrensha
Copy link
Member

ReaderOptions.traversal_limit_in_words and ReaderOptions.nesting_limit exist to deal with untrusted data. When you have a message::Builder, you must have constructed the data yourself, so it's safe to set both of those options to max_value(). I think that's the right thing to do here.

Note that it also might make sense to convert a message::Builder to a message::Reader without going to a message::TypedReader, and it might make sense to have a message::TypedBuilder in the future. So I think it makes more sense to have separate methods: message::Builder::into_reader() and message::Reader::into_typed(). We may also want to add an message::TypedReader::into_inner() message to allow recovering the reader.

@bspeice
Copy link
Contributor Author

bspeice commented Jan 27, 2018

Idiomatic Rust question then - Should it just be TypedReader From both message::Builder and message::Reader? Saves one extra function call (builder.into_reader().into_typed()) and uses the Rust API's for it.

@dwrensha
Copy link
Member

I prefer adding methods with self-explanatory names like into_reader() and into_typed(). We could impl From on top of those methods, if we want that too.

Uses the recommended `into_typed` and `into_reader` methods
@bspeice
Copy link
Contributor Author

bspeice commented Jan 31, 2018

All the above are done. Let me know if there's anything else, sorry about the wait on this one.

@dwrensha dwrensha merged commit 10523a6 into capnproto:master Jan 31, 2018
@dwrensha
Copy link
Member

Thanks!

@dwrensha
Copy link
Member

Hm... it occurs to me that the rest of the codebase uses methods named as_reader() instead of into_reader() for this kind of situation. I wonder if it makes sense to use that naming scheme here also, for the sake of consistency (even if into_*() is more idiomatic in the wider Rust community).

@dwrensha
Copy link
Member

into_reader() had an unnecessary type parameter: 5bb2b41

@bspeice bspeice deleted the bspeice-patch-1 branch January 31, 2018 03:40
@dwrensha
Copy link
Member

dwrensha commented Feb 1, 2018

After further reflection, I think the into_ prefix is correct here. It signifies that the ownership is being transferred. In constrast, a prefix of as_ usually signifies that something is being borrowed. It's true that many as_reader methods in capnproto-rust (e.g. this one) technically take self by value, but their usual intended use case is more like borrowing, in particular when they are used in conjunction with a borrow() method. (See this blog post for more discussion of these methods.)

vhdirk pushed a commit to vhdirk/capnproto-rust that referenced this pull request May 8, 2019
Convert (consume) Builder into TypedReader
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

2 participants