tafia / quick-protobuf

A rust implementation of protobuf parser
MIT License
452 stars 87 forks source link

Add no_std support #145

Open mullr opened 5 years ago

mullr commented 5 years ago

This is a series of changes to add no_std support to quick-protobuf. It is unfortunately a breaking change; this can't be avoided, as the existing API directly uses the std-only std::io::Write trait. But it only requires regenerating the stubs; after that, everything works the same way it used to.

There are two major changes here:

mullr commented 5 years ago

Appveyor failure looks unrelated:

'cargo-fmt.exe' is not installed for the toolchain 'stable-x86_64-pc-windows-msvc'
mullr commented 5 years ago

Travis passes on stable; the other failures appear to be because the test script always wants to be on stable.

nerdrew commented 5 years ago

This diff is really large :) Mind breaking the "cleanup" commits into a separate PR so we can quickly review and merge them? I think I have many of the same cleanups on a branch somewhere that I've been meaning to PR. Did you run cargo clippy by any chance for the cleanups?

nerdrew commented 5 years ago

What are people's thoughts on gitignore-ing all the generated files? I find they cause PRs to be quite a bit larger than they should be. I like to see some of the generated code if a PR is changing the code generation, but I don't know if we need to check it all in. Thoughts?

mullr commented 5 years ago

What are people's thoughts on gitignore-ing all the generated files? I find they cause PRs to be quite a bit larger than they should be. I like to see some of the generated code if a PR is changing the code generation, but I don't know if we need to check it all in. Thoughts?

+1 on this; it was pretty awkward dealing with all the generated code diffs in this commit.

mullr commented 5 years ago

This diff is really large :) Mind breaking the "cleanup" commits into a separate PR so we can quickly review and merge them? I think I have many of the same cleanups on a branch somewhere that I've been meaning to PR. Did you run cargo clippy by any chance for the cleanups?

That's a good idea. I'll make a separate PR for that and base this on that branch. I didn't clippy, but I will.

nerdrew commented 5 years ago

@tafia Do you have an opinion on gitignoring the generated proto rust files? If you don't object, I'll open a PR.

mullr commented 5 years ago

@mullr, apart from the rust_gen_arrayvec comment, I have a another one: I'd like the default behavior to be as close as today if possible, which means using std::io::Read and not having WriterBackend per default. The reason is to keep the generated code as lean as possible.

That would add a fair bit of complexity to the codegen; you'd have to choose regular mode, and no_std mode. I don't think you'd gain very much at all from it, either: all of the write calls are using static dispatch (not trait objects), so the everything will get monomorphized. S if the caller is using the std::io backend, the compiler will end up generating the exact same code.

Similarly I believe we should not have the arrayvec crate in default features. (default should be std only).

Agreed. I tried to do that, but I couldn't figure out how to enable non-default features for tests. Is there a way?

tafia commented 5 years ago

That would add a fair bit of complexity to the codegen.

Wouldn't it be just replace WriterBackend with std::io::Write and change the use ... at the top? In terms of knowing when to use what, it could be either an explicit flag or by checking features.

But I agree that this is not super important. @nerdrew, any preference?

I couldn't figure out how to enable non-default features for tests

Haven't tried but something like cargo test --no-default-features --feature xyz should work?

nerdrew commented 5 years ago

Wouldn't it be just replace WriterBackend with std::io::Write and change the use ... at the top? In terms of knowing when to use what, it could be either an explicit flag or by checking features.

This makes sense to me. I think you'd need a flag for pb-rs and then a feature for quick-protobuf.

By default I like keeping std and std::io::{Error, Read, Write} for ease of use in the common (or should I say... "standard") case.

nerdrew commented 5 years ago

Question about ArrayVec: it looks like you can use Vec in a no_std environment. I don't really know much about it, but this doc says you can if you can add alloc and collections. When people say no_std do they also usually mean no alloc and no global allocator?

mullr commented 5 years ago

Question about ArrayVec: it looks like you can use Vec in a no_std environment. I don't really know much about it, but this doc says you can if you can add alloc and collections. When people say no_std do they also usually mean no alloc and no global allocator?

Yes, that's right.

mullr commented 5 years ago

Ok, I've rebased this branch on master, with one small change: the field option is now '(rust_max_length)', so it could in principle be used for other kinds of fields, works with protoc, and could in principle be registered as an official extension.

w.r.t. the other requested design changes: I'm out of time to work on this. At this point it's good enough for the use case I have, and I can't really justify any more time spent on reworking it. You guys are welcome to do with it what you want. Of course I'd be thrilled if there was upstream support for no_std.

tafia commented 5 years ago

w.r.t. the other requested design changes: I'm out of time to work on this. At this point it's good enough for the use case I have, and I can't really justify any more time spent on reworking it. You guys are welcome to do with it what you want. Of course I'd be thrilled if there was upstream support for no_std.

Thanks a lot for your work!

xoloki commented 5 years ago

Hi @nerdrew @tafia, I'm thinking about taking this branch, removing the arrayvec code, and using the alloc collections when in no_std mode. This removes the need for the length annotations as well. Would you be interested in a PR with those changes?

tafia commented 5 years ago

Would you be interested in a PR with those changes?

Certainly!

nerdrew commented 4 years ago

Looks like this can be closed? no_std support looks like it was merged separately.

MathiasKoch commented 1 year ago

I don't think this should be closed entirely yet, as this PR (If i'm reading it correctly) not only provides no_std, but also no_alloc usage?

This is a huge advantage for embedded use-cases, where an allocator is broadly not available or not desired.

quentinmit commented 1 year ago

What's blocking this PR? Is it just the merge conflicts? It sounds like all that's left for no_alloc support is to merge the rust_max_length option with arrayvec support. (And yes, no_alloc is very important for embedded applications.)