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

Make pest_generator / pest_derive no_std compatible. #513

Merged
merged 6 commits into from
Oct 10, 2021
Merged

Make pest_generator / pest_derive no_std compatible. #513

merged 6 commits into from
Oct 10, 2021

Conversation

01mf02
Copy link
Contributor

@01mf02 01mf02 commented May 3, 2021

This is the follow-up pull request to #498.
It adds "std" feature flags for both pest_generator and pest_derive.
Currently, the correct function of this can only be tested by running cargo test --no-default-features in the derive directory.
Running cargo test --no-default-features in the root directory unfortunately does not run the tests in no_std mode.

@CAD97
Copy link
Contributor

CAD97 commented May 14, 2021

Again, I prefer crates being unconditionally #![no_std] and then conditionally doing extern crate std; Other than that, everything looks good to me.

I was going to say that you can unconditionally use ::core as a path for core items, but since we're emitting tokens with call-site spans, they'll have call-site edition hygiene, and as such won't have core available for edition=2015 without extern crate core. At least as I understand it, anyway.

@01mf02
Copy link
Contributor Author

01mf02 commented May 15, 2021

Hi @CAD97, I made the tests unconditionally no_std as you suggested, thanks for spotting this.

I also tried using ::core unconditionally, but unfortunately did not get this to work. The original patch (#243) used a similar technique.

@01mf02
Copy link
Contributor Author

01mf02 commented May 15, 2021

Oh, I now remember why I originally did not make the tests unconditionally #![no_std]:
The reason for this is that #![no_std] makes ::core accessible.
Therefore, indeed, somebody could change the code in generator.rs to use ::core::option::Option regardless of std and believe that everything is fine, because all tests (especially from derive) would still pass.
However, any regular user of the library that does not use #![no_std] would then see failures such as:

error[E0433]: failed to resolve: maybe a missing crate `core`?
  --> derive/tests/grammar.rs:17:10
   |
17 | #[derive(Parser)]
   |          ^^^^^^ not found in `core::option`

That is why I think it is not a good idea to unconditionally use #![no_std] in the derive tests.

@01mf02
Copy link
Contributor Author

01mf02 commented May 27, 2021

Is there anything that blocks the merge of this PR?
Should I revert my changes that made the tests unconditionally #![no_std]?

@01mf02
Copy link
Contributor Author

01mf02 commented Jul 16, 2021

@CAD97, @dragostis, what is the status on this?

@luojia65
Copy link

luojia65 commented Aug 6, 2021

Should this be merged? no_std pest can be used on operating system kernels that will not provide an std crate.

@01mf02
Copy link
Contributor Author

01mf02 commented Oct 4, 2021

I now reverted my commit that made the derive tests unconditionally #![no_std], as explained above.
From my point of view, this should be merged. :)

@CAD97
Copy link
Contributor

CAD97 commented Oct 4, 2021

Sorry that this has sat for so long. If you fix the conflict, I think I can merge this. We're still waiting on @dragostis for a publish, though.

@01mf02
Copy link
Contributor Author

01mf02 commented Oct 4, 2021

Ok, I fixed the conflict. From my POV, this is ready to merge.

@CAD97
Copy link
Contributor

CAD97 commented Oct 10, 2021

LGTM

bors: r+

@bors
Copy link
Contributor

bors bot commented Oct 10, 2021

@bors bors bot merged commit bc0dd1b into pest-parser:master Oct 10, 2021
@01mf02
Copy link
Contributor Author

01mf02 commented Oct 11, 2021

Amazing! Thank you for merging this.

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.

4 participants