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

Add channels example #8611

Closed
wants to merge 1 commit into from
Closed

Add channels example #8611

wants to merge 1 commit into from

Conversation

twetzel59
Copy link
Contributor

Added an example that shows
how to use channels in a typical
multithreaded scenario.

Added an example that shows
how to use channels in a typical
multithreaded scenario.
@twetzel59
Copy link
Contributor Author

This is just my second PR for Nim, so let me know if anything isn't quite right.

The example aims to be general but useful by explaining what I think most people might often do with channels.

@timotheecour
Copy link
Member

timotheecour commented Aug 12, 2018

could that code block be turned into a runnableExamples: ? that way it's guaranteed to always compile/run

@twetzel59
Copy link
Contributor Author

@timotheecour, can runnableExamples work at module level? I wasn't quite sure how to use it l, so I searched the Nim repo and with a quick look I only saw it inside procs.

Many other modules have module level examples in a comment. Do they not use runnableExamples because they haven't been updates, or for a good reason?

@Araq
Copy link
Member

Araq commented Aug 13, 2018

This example mixes channels with threadpool and that's unwise. Also how the documentation of channels is generated is bad and needs to be changed.

@twetzel59
Copy link
Contributor Author

Would it be better to use pure threads, or single threading instead?

@Araq
Copy link
Member

Araq commented Aug 14, 2018

Better show how to use plain threads with these channels.

@timotheecour
Copy link
Member

@twetzel59

can runnableExamples work at module level?

just filed:

IMO it'd be higher priority to fix this, so that you can add your example in top-level runnableExamples instead of adding more legacy code that'll later need to be changed to runnableExamples block.
But doesn't have to be you :-) If no-one volunteers I can look into it, but I have my hands full with a ton of other PR's at the moment.
I don't think it's a hard bug to fix

lib/system/channels.nim Show resolved Hide resolved
@timotheecour
Copy link
Member

@timotheecour, can runnableExamples work at module level? I wasn't quite sure how to use it l, so I searched the Nim repo and with a quick look I only saw it inside procs.

it now works in devel

@twetzel59
Copy link
Contributor Author

@timotheecour I have a question about the Git workflow here. My fork is quite old and doesn't have that fix yet. I assume I need to sync the fork.

Will doing so introduce an unwanted merge commit, or is that okay? Sorry, I'm a bit new to working on large projects :)

@timotheecour
Copy link
Member

we really want to avoid merge commits, as discussed in another issue. Just do: git pull --rebase origin devel and then, if there are conflicts, resolve them. If you're having any issues in the process I'm happy to help

@Araq
Copy link
Member

Araq commented Dec 21, 2018

No progress, the example still uses the threadpool.

@Araq Araq closed this Dec 21, 2018
chr-1x added a commit to chr-1x/Nim that referenced this pull request Jan 20, 2020
This PR revives nim-lang#8611 since the channels module still does not have an
example of usage. I attempted to fix the outstanding issue that caused
the previous PR to be closed, namely implementing the example on top of
threads instead of threadpool. I also added an additional example of
using sharedAlloc0 to create channel pointers based on the discussion in
 nim-lang#4748. Please let me know if there is anything that needs to be
changed!
@chr-1x chr-1x mentioned this pull request Jan 20, 2020
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