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

Consider swapping permutohedron::control::Control & friends with std::ops::ControlFlow #10

Open
schneiderfelipe opened this issue Feb 23, 2024 · 3 comments

Comments

@schneiderfelipe
Copy link
Collaborator

This (in fact the whole file)

/// Control flow for callbacks.
///
/// `Break` can carry a value.
#[derive(Copy, Clone, Debug)]
pub enum Control<B> {
Continue,
Break(B),
}

could be swapped with std::ops::ControlFlow, available since Rust 1.55.0.

But signatures like

permutohedron/src/lib.rs

Lines 44 to 46 in afdb943

pub fn heap_recursive<T, F, C>(xs: &mut [T], mut f: F) -> C
where F: FnMut(&mut [T]) -> C,
C: ControlFlow,

would become (something like)

pub fn heap_recursive<T, F, B, C>(xs: &mut [T], mut f: F) -> std::ops::ControlFlow<B, C>
    where F: FnMut(&mut [T]) -> std::ops::ControlFlow<B, C>,

or maybe even

pub fn heap_recursive<T, F, B, C>(xs: &mut [T], mut f: F) -> Result<C, B>
    where F: FnMut(&mut [T]) -> std::ops::ControlFlow<B, C>,

since std::ops::Try is not yet stable (rust-lang/rust#84277); that would be a breaking change.

What do you think @bluss?

@bluss
Copy link
Owner

bluss commented Mar 7, 2024

I think some of the finesse is removed when C=() is no longer allowed?

@schneiderfelipe
Copy link
Collaborator Author

I think some of the finesse is removed when C=() is no longer allowed?

This is very true. In any case, we can get rid of control::Control by making use of std::ops::ControlFlow<B, ()>. I'll make a PR to see how it looks like.

@schneiderfelipe
Copy link
Collaborator Author

I think some of the finesse is removed when C=() is no longer allowed?

On a second thought, even forcing the usage of std::ops::ControlFlow as the output of every closure would not remove any finesse (one could return std::ops::Continue(()) forever), but would severely hurt ergonomics (e.g., functions returning Result<(), E> would have to be wrapped, etc.).

Even using std::ops::Try (when it stabilizes) would disallow C = () (as () is currently not std::ops::Try). So #11 might be a nice middle ground, swapping control::Control for std::ops::ControlFlow, but still as an implementation of control::Control trait.

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

No branches or pull requests

2 participants