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

Sendable support #30

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Conversation

seanmrich
Copy link

Gen modified to be Sendable.

Copy link
Member

@stephencelis stephencelis left a comment

Choose a reason for hiding this comment

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

Overall this looks good! I think all the @Sendable annotations on the methods/functions aren't needed, though. Can you remove them or show where things go wrong without them? Then I'll kick off CI against this branch.

@@ -13,6 +13,7 @@ public struct Gen<Value> {
/// - Parameter rng: A random number generator.
/// - Returns: A random value.
@inlinable
@Sendable
Copy link
Member

Choose a reason for hiding this comment

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

I think this can be removed. Do you have a situation in which it's required?

Suggested change
@Sendable

Copy link
Author

Choose a reason for hiding this comment

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

That’s good feedback. I didn’t include those annotations at first, but the compiler highlighted an issue with composability of the methods. I’m on vacation at the moment so I don’t have my mac in front of me, but I remember there were a couple of times in the library code where a function was passed as a parameter and needed to be Sendable. Looks like ‘shuffled’ was one of them. Rather than just annotate a couple methods I included the annotation on all similar methods. I could change those cases to use an explicit closure and call the method from inside the body if you prefer. I just didn’t see a downside to including the annotation, especially since it was used in the library itself.

Copy link
Member

Choose a reason for hiding this comment

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

@seanmrich I think those instances could potentially be refactored, but it'd help if you could share whenever you're back, no rush 😄

@@ -27,6 +28,7 @@ public struct Gen<Value> {
///
/// - Returns: A random value.
@inlinable
@Sendable
Copy link
Member

Choose a reason for hiding this comment

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

This too.

Suggested change
@Sendable

@@ -39,6 +41,7 @@ extension Gen {
/// - Parameter value: A constant value.
/// - Returns: A generator of a constant value.
@inlinable
@Sendable
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
@Sendable

@@ -48,31 +51,24 @@ extension Gen {
/// - Parameter transform: A function that transforms `Value`s into `NewValue`s.
/// - Returns: A generator of `NewValue`s.
@inlinable
public func map<NewValue>(_ transform: @escaping (Value) -> NewValue) -> Gen<NewValue> {
@Sendable
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
@Sendable

extension Gen {
/// Transforms a generator of `Value`s into a generator of `NewValue`s by transforming a value into a generator of `NewValue`s.
///
/// - Parameter transform: A function that transforms `Value`s into a generator of `NewValue`s.
/// - Returns: A generator of `NewValue`s.
@inlinable
public func flatMap<NewValue>(_ transform: @escaping (Value) -> Gen<NewValue>) -> Gen<NewValue> {
@Sendable
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
@Sendable

@seanmrich
Copy link
Author

Hopefully this commit meets the spirit of what you were looking for.

@seanmrich
Copy link
Author

Sorry about the CI failures. I have to admit I've got little experience with workflows so I had to read up on it. Should be good to go now.

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