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

no_std #7

Merged
merged 7 commits into from
Jun 27, 2023
Merged

no_std #7

merged 7 commits into from
Jun 27, 2023

Conversation

Easyoakland
Copy link
Contributor

Removed unnecessary bounds on Location and implemented no_std compatibility.

@jsdw
Copy link
Owner

jsdw commented Jun 27, 2023

Thanks for your PR! I've added back the location bounds (PartialEq and Debug) for now; the original reason for PartialEq existing was so that somebody could always compare two locations to see if they are the same, and Debug seems useful for debugging locations.

But I'll have a think; maybe removing them makes more sense because people can enforce them again if needed with their own blanket trait or something like that, and as you noted they aren't needed in the library or examples.

But for now let's just get no-std in :)

@jsdw jsdw merged commit 7e3fef8 into jsdw:master Jun 27, 2023
@Easyoakland
Copy link
Contributor Author

Easyoakland commented Jun 28, 2023

Generic bounds (such as Debug and PartialEq) can be implemented on a user's implementation of Location without requiring all Locations to have Debug and PartialEq. I also wanted to remove Clone but that involved more changes to the slice impl bounds than I wanted to do in this commit. I don't see why it is advantageous to limit the number of things that can implement Location beyond what is necessary. If I want a PartialEq Location then I will implement it on my type or bound by Location + PartialEq in my function.

@jsdw
Copy link
Owner

jsdw commented Jun 28, 2023

Probably the main/only reason for the extra bounds is just user friendliness; ie somebody can write a function that takes an &mut impl Tokens<Item = char>, and by default there are some things that the user can always do given that, like clone, debug and compare locations. (otherwise you end up with more annoying type signatures everywhere to "opt-in" to extra things you need, or concrete types everywhere, which you can of course do if you wish, but I didn't want to push people in that direction).

I def understand the desire to be more precise in the bounds and make these things opt-in instead though, so probably there's a nice way to meet in the middle; maybe just something like "write a blanket trait impl to add any extra bounds" with an example of this.

When I have time I'll think about it more; currently travelling!

@Easyoakland
Copy link
Contributor Author

Easyoakland commented Jun 29, 2023

  1. I disagree with the idea that loose bounds for convenience is actually better that accurate bounds. Convenience can be worked around. Inaccurate bounds can make it impossible to use the library. Further, what defines where the extra bounds should end? Why not Eq and Ord while we're defining extra bounds?
  2. Regardless, I tried writing an alias trait.
    I got here:
pub trait DebugTokens: Tokens
where
    Self::Location: PartialEq + Debug + Clone,
{
}
impl<T> DebugTokens for T
where
    T: Tokens,
    T::Location: PartialEq + Debug + Clone,
{
}

and encountered this issue: rust-lang/rust#20671. Something to note if you attempt similarly.
3. If the removed bounds occurred then worst case:

fn f(toks: &mut impl Tokens<Item = char>) {
    /* content */
}

becomes

fn f(toks: &mut impl Tokens<Item = char, Location = impl PartialEq + Debug + Clone>) {
    /* content */
}

in the places where that restriction was actually needed.
I doubt that a production parser will need the Debug bound and there is already the is_at_location method for comparing Locations. Meaning realistically it will probably only become

fn f(toks: &mut impl Tokens<Item = char, Location = impl Clone>) {
    /* content */
}

at this point I could see the argument that Clone is useful and ubiquitous enough (very few things shouldn't impl Clone) that it makes sense to require it for Location for convenience's sake. I'd still disagree, but it would be more reasonable to me.

@Easyoakland Easyoakland deleted the no_std branch August 16, 2023 08:37
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.

2 participants