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

Some tweaks to the @tasks macro #62

Merged
merged 5 commits into from
Mar 1, 2024
Merged

Some tweaks to the @tasks macro #62

merged 5 commits into from
Mar 1, 2024

Conversation

MasonProtter
Copy link
Member

@MasonProtter MasonProtter commented Mar 1, 2024

Hey @carstenbauer, some of these would have been annoying to do in the code review section so I figured I'd just do it in a PR to your PR. Very meta.

Summary of suggested changes here, feel free to reject any of them:

  • Move the internals of the macro to the Implementation module so they don't appear as public API.
  • Make it so that @tasks only accepts a single argument. Not zero or 1 arguments
  • Make it so that @tasks will error if the users types things like
@tasks for _ in 1:N, _ in 1:M
   body
end

(before it macroexpanded to something that'd throw a confusing syntax error during lowering)

  • (nitpicky) before the @init things inserted code like (OhMyThreads.OhMyThreads).TaskLocalValue{T}((()->... into the AST, so I made it instead do OhMyThreads.TaskLocalValue{T}((()->....
  • I removed the code you commented out for handling keyword argument options. If you think we might want this later, I can restore it.

Other than these minor things, this looks fantastic. Thanks for taking it on!

@carstenbauer
Copy link
Member

I like it. I think we're ready to merge both PRs.

@MasonProtter MasonProtter merged commit 2d9acb4 into cb/macro Mar 1, 2024
8 of 9 checks passed
@MasonProtter MasonProtter deleted the mp/macro-review branch March 1, 2024 11:37
MasonProtter added a commit that referenced this pull request Mar 1, 2024
* try implement threaded macro

* fix conflict

* reduce -> reducer; support begin end block for tasklocal macro

* wrap in let block

* Update src/macro.jl

Co-authored-by: Mason Protter <[email protected]>

* rename macros

* add support for settings block

* hygiene

* fix conflict

* only take setting through set macro, i.e. drop keyword arguments

* comment out unused code

* some tests

* improve tests a little bit

* add collect=true -> tmap option + more tests

* tmp

* fix conflict

* basic docstring

* docstring changes

* add macro api examples to docs

* include tasks macro example in tls

* add bumper to tls doc page

* change comment

* docsstrings for set and init + README

* minor

* adjust changelog (bump version number)

* Some tweaks to the `@tasks` macro (#62)

* move macro body to the internals module

* only accept one argument, error if the user gives a complex loop assignemnt

* remove double quoting, reformat comment

* remove outdated code

* re-org

---------

Co-authored-by: Mason Protter <[email protected]>
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