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

[WIP] No std #243

Closed
wants to merge 2 commits into from
Closed

[WIP] No std #243

wants to merge 2 commits into from

Conversation

Restioson
Copy link

@Restioson Restioson commented May 26, 2018

Fixes #240.

TODO:

  • Test for whether it works in no std

@Restioson Restioson changed the title No std [WIP] No std May 26, 2018
@@ -83,7 +85,7 @@ impl<R: RuleType> BitOr for Operator<R> {
/// [1]: https://en.wikipedia.org/wiki/Operator-precedence_parser#Precedence_climbing_method
#[derive(Debug)]
pub struct PrecClimber<R: RuleType> {
ops: HashMap<R, (u32, Assoc)>
ops: BTreeMap<R, (u32, Assoc)>
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm curious about this change. It seems like this isn't relevant for no_std. Why the change?

Copy link
Author

Choose a reason for hiding this comment

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

It is relevant, HashMap isn't in alloc (it uses rand or something).

Copy link
Author

Choose a reason for hiding this comment

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

Maybe it could be cfg'd though, if it were any slower (is it? benching required)

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that the best approach here is to limit the Vec in the contractor to 256. (assert + document with # Panics section) Then use a Box<[Assoc; 256]> to store the ops.

Copy link
Contributor

Choose a reason for hiding this comment

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

We'll definitely want to test to see if there's a notable difference. I suspect for most of our use-cases it'll be similar enough.

Copy link
Contributor

@dragostis dragostis left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. Things look good, however, I might have some second thoughts on the complexity that this adds.

Adding no-std is definitely a good way to move forward, however. @jstnlef what do you think about changes?

pub use core::*;
pub use core::{fmt, str};

/// Cheap-ish knockoff of std::erorr. Just does what we need it to, nothing more.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm wondering if this is compatible with failure. Truth be told, I guess it doesn't matter too much in the context of no-std.

Copy link
Author

Choose a reason for hiding this comment

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

Hmm... How could we test?

macro_rules! generate_rule {
($name: ident, $pattern: expr) => {
quote! {
#[inline]
#[allow(dead_code, non_snake_case, unused_variables)]
fn $name(state: Box<::pest::ParserState<Rule>>) -> ::pest::ParseResult<Box<::pest::ParserState<Rule>>> {
fn $name(
state: Box<::pest::ParserState<Rule>>
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe there should be a macro that returns the type here behind some cfgs in order to eliminate the duplication.

Copy link
Author

Choose a reason for hiding this comment

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

Yeah perhaps

@@ -24,3 +24,6 @@ syn = "^0.13"
codecov = { repository = "pest-parser/pest" }
maintenance = { status = "actively-developed" }
travis-ci = { repository = "pest-parser/pest" }

[features]
no_std = []
Copy link
Contributor

Choose a reason for hiding this comment

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

What I dislike about this solution is that you need to specify no_std in both crates in that it let's the end user use an incompatible configuration (e.g., std + no-std), while providing shallow feedback. (some compile error down the line most likely) Is there any way to specify this in one single place?

Copy link
Author

Choose a reason for hiding this comment

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

Actually I probably can make them depend on eachother. I'll try that.

@jstnlef
Copy link
Contributor

jstnlef commented May 31, 2018

Also looks like the build is broken. Seems like some changes need to happen to pest_vm.

@Restioson
Copy link
Author

Yeah, that's a bit weird. Strangely it Works On My Machine (as you can see from the screenshot), but I'll look at that soonish. Unfortunately I am writing exams right now and for the next few weeks so I may not be able to get to it that soon, but after exams I will.

#![cfg_attr(feature = "no_std", feature(alloc))]
#![cfg_attr(feature = "no_std", feature(slice_concat_ext))]

#![cfg(feature = "no_std")]
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like this exclamation mark should be removed.

Copy link
Author

Choose a reason for hiding this comment

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

Yes, haha.

@wirelyre
Copy link
Contributor

@dragostis Does Travis need to be updated to test both regular and no_std configurations?

@Restioson
Copy link
Author

@wirelyre unfortunately to get rustc to properly face the fact that there is no std you have to link it into some kind of binary, so we'd somehow need to make it do that.

@dragostis
Copy link
Contributor

@wirelyre Yes, but that shouldn't be a huge issue. I would simply add another cargo build/cargo test run for the no_std case. This should be part of this same PR, I'd say.

@Restioson Travis issue might stem from the fact that it's running the tests on stable and you might have a different configuration.

@Restioson
Copy link
Author

Probably I forgot to test it with std enabled.

@Restioson
Copy link
Author

OK - I'm done exams so hopefully I'll be able to look at this soon.

@sjmackenzie
Copy link

How's the status of this PR? Would be fantastic to have it in.

@Restioson
Copy link
Author

Hi! Unfortunately, it's been years since I've worked on this, and I imagine that both the rust no_std ecosystem as well as pest itself has evolved beyond this change. Therefore, I'll close this now as it'd probably be easier to implement this from scratch from HEAD than to update it from my 4 year old fork 😅 If someone else wants to, feel free to reuse any code from this or the approach, but personally I don't have the bandwidth to see this through right now.

@Restioson Restioson closed this Jun 5, 2022
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.

No std?
5 participants