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

Add no_std support #158

Merged
merged 33 commits into from
May 17, 2020
Merged

Add no_std support #158

merged 33 commits into from
May 17, 2020

Conversation

jpopesculian
Copy link
Contributor

Haha let's try this a 3rd time... Building off of @xoloki's #148 work, I'd like to get this finalized and merged in. Let me know what you guys need to get this done

mullr and others added 30 commits September 9, 2019 15:07
This replaces all public uses of the Writer trait with the new WriterBackend,
which defines only the interaction points which are actually used. It also
introduces the BytesWriter struct as a new implementation of WriterBackend that
will work in no_std, along with the serialize_into_slice convenience fn.

This is technically a breaking change, as it will require stubs to be
regenerated.
- Add the `std` feature to the quick-protobuf runtime crate
- Put uses of Vec and std::io::Write behind the `std` feature
- Change other uses of `std` to `core`
- Add the `quick-protobuf/no-std-example` compile test
@jpopesculian jpopesculian requested review from nerdrew and tafia January 24, 2020 17:04
@jpopesculian
Copy link
Contributor Author

@nerdrew @tafia what's the status on this?

Copy link
Owner

@tafia tafia left a comment

Choose a reason for hiding this comment

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

Thanks a lot for this PR, I really like it.
I have one small comment about KVMap which I'd like to be removed if possible.

Sorry for the super long delay before the review!

@@ -312,7 +312,7 @@ impl FieldType {
format!("{}{}{}", m.get_modules(desc), m.name, lifetime)
}
FieldType::Map(ref key, ref value) => format!(
"HashMap<{}, {}>",
"KVMap<{}, {}>",
Copy link
Owner

Choose a reason for hiding this comment

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

I believe most people won't use no_std and they'd rather use regular std::HashMap.
I understand that the distinction is done at import time but I'd like things to be as simple as possible for std users.

@@ -312,7 +312,7 @@ impl FieldType {
format!("{}{}{}", m.get_modules(desc), m.name, lifetime)
}
FieldType::Map(ref key, ref value) => format!(
"HashMap<{}, {}>",
"KVMap<{}, {}>",
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
"KVMap<{}, {}>",
"{}<{}, {}>",
if config.no_std { "KVMap" } else { "HashMap" },

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey @tafia, sorry for the delay in the reply! Thanks for taking a look 😄 I added the KVMap because the rust_type function doesn't have access to the config object (and I didn't want to change the call definition for a bunch of function), and the alias shouldn't restrict with what the actual person uses (other than reflecting in the docs). Obviously, I'd be happy to change it if you want, but I wanted to check with you first. I could of course also alias BTreeMap to HashMap but that seems a little hacky...

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 checked, the alias, actually does not reflect in the docs because its private. It's really pretty invisible to the user of the struct...

@therealprof
Copy link
Contributor

Can we please get this merged some time? 😅

Copy link
Owner

@tafia tafia left a comment

Choose a reason for hiding this comment

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

Great

@tafia
Copy link
Owner

tafia commented May 17, 2020

Sorry for the very long delay. Merging it once the CI tests pass.
Will deploy a new version just after

@tafia tafia merged commit 5325627 into tafia:master May 17, 2020
@therealprof therealprof mentioned this pull request May 17, 2020
@tafia
Copy link
Owner

tafia commented May 17, 2020

Deployed v0.9.0 of pb-rs

@therealprof
Copy link
Contributor

Causes #165

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.

5 participants