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

replace for with .fold() #45

Closed
wants to merge 1 commit into from
Closed

Conversation

chris-ha458
Copy link
Contributor

This was kinda hacked into #44 as an after thought, but since it seems that might not be a good idea at this point, I extracted the fold() changes.

Whether this is more idiomatic code is subjective. Since subjective aspects would come down to the preference of the maintainer, I won't go too much into it. I will go into objective parts.

  • for isn't strictly necessary since each closure, except for the Counter access has no other side effects
  • this construction would make it easier for functional extensions, such as rayon::into_par_iter() (Although certain concurrency or atomicity related changes would need to be made for this)

@coriolinus
Copy link
Owner

Agree that for isn't strictly necessary here, but without getting into the question of what is more idiomatic, I'd prefer the for construction over fold for this case.

It's not clear what you mean by saying that this makes functional extensions easier. It's easy to imagine a rayon feature which when enabled does impl rayon::iter::ParallelIExtend for Counter, but the ParalllelExtend implementation would never just defer to the FromIterator implementation. Can you sketch out more precisely you expect this to make future implementation easier?

@chris-ha458
Copy link
Contributor Author

but the ParalllelExtend implementation would never just defer to the FromIterator implementation. Can you sketch out more precisely you expect this to make future implementation easier?

I thought I'd do a simple rayon par_into_iter(), but it seems like it wouldn't be easy or even possible with the current bounds.
Another impl with different relevant bounds satisfied would be necessary and I get that even if such a construction was used to extend it functionally, it likely not look like the current version.

I might ponder on this a bit before closing, but feel free to close it earlier if you don't feel like there is anything here.

@coriolinus coriolinus closed this Sep 28, 2023
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