-
Notifications
You must be signed in to change notification settings - Fork 507
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 the join!
macro
#406
base: main
Are you sure you want to change the base?
Add the join!
macro
#406
Conversation
You can let macro hygiene deal with the temporaries like this: (playground) macro_rules! join {
...
( @ $( let $lhs:ident = $rhs:expr ; )+ ) => {
{
let join!( < $( $lhs , )+ ) = join!( > $( $rhs , )+ );
($( $lhs ),+) // flattened tuple
}
};
( @ $( let $lhs:ident = $rhs:expr ; )* $x:expr $( , $xs:expr )*) => {
join! { @
$( let $lhs = $rhs ; )*
let lhs = $x;
$($xs),*
}
};
( $x:expr $( , $xs:expr )* ) => {
join! { @ $x $( , $xs )* }
};
} They're all called Might want to use a different sigil than |
@cuviper updated! |
Can you elaborate the problem with that? I just tried changing |
I think I was trying to do it without the Anyways, I've added |
Can you rebase this for the CI fixes? @nikomatsakis I'd also like to know what you think of this interface. I think the plain |
The `join!` macro forks and joins many expressions at once. This is easier to use than deeply nesting calls to `rayon::join`.
Rebased. Note that the |
That's a fair point! I'm not opposed to using We could just take plain join! {
let x = 1;
let y = 2;
let z = 3;
}
// becomes:
let (x, (y, z)) = join(|| 1, join(|| 2, || 3)); But that's less flexible than taking a user's closure, and also implies a sequence that doesn't really exist -- e.g. I don't have any great alternatives. |
Yeah, and I think defining the closures is actually necessary to support, so people can
I didn't think of that. Not sure how to address it. |
Mmm -- is there ever a time that they need to move (modulo compiler bugs)? I don't think there should be (and if there really was you can always force a move for individual variables). The main reason that one needs to write |
Usually these kinds of macros are called something like "parallel blocks" -- is the name |
At least in the case of |
@cuviper does it support that form, or do you have to use |
Ah, I see from the macro it supports both. Seems like the docs should mention that more prominently, or maybe I just missed it. |
Oh, never mind. I see it now. =) |
OK, so I think we agree that the "variadic" Maybe the One other thing that occurred to me it looks a little weird that we're not creating a scope for those Finally, since |
@cuviper interesting point about moving |
|
||
// Necessary for the tests using macros that expand to `::rayon::whatever`. | ||
#[cfg(test)] | ||
mod rayon { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can use $crate
in the macro and remove this helper module.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point -- the macro should use $crate anyway.
So @cuviper and I revisited this today in our Rayon sync meeting. Some thoughts: As far as the bikeshed goes, I'm inclined to this form: rayon::parallel_let! {
a = ...;
b = ...;
} Alternatives:
Putting The new macro system makes namespacing a non-issue. Yay! As for the process, I'd be ok accepting this PR because of grandfathering. That said, I'd love to see this reframed as an RFC, laying out some of the motivation and getting a bit broader feedback. But I don't want to ask too much of poor @fitzgen. As @cuviper said, "Hey @fitzgen, remember that PR that stalled? please now use this bigger process for it..."... |
Yes to that! I think
Possibly, but unsure. This might look strange with semicolons: rayon::par_let!(a = 10, b = 20, c = 30); Another thing to take into consideration when gauging the syntax is that there might be pattern matching and types thrown in: rayon::par_let! {
a: isize = foo(10),
(a, b): (u32, String) = bar(20),
c = baz(30),
} Not sure how I feel about that. Looks messy and unlike any other Rust syntax I've seen. The following looks much better, but suffers from the scoping confusion you mentioned: rayon::parallel! {
let a: isize = foo(10);
let (a, b): (u32, String) = bar(20);
let c = baz(30);
} |
Currently
@nikomatsakis suggested we might be able to hack this by accepting either Also, I don't know if this was intentional, but your examples have |
As a way forward, how would folks feel about just adding |
works for me |
I misunderstood, they meant |
Yes. The closure may want to take ownership of some object (e.g. to take it apart). |
I would note that there is no plenty of precedent from futures, too: https://docs.rs/futures/0.3.15/futures/macro.join.html |
@rocallahan since you asked in #865, are you interested in restarting this? |
I just hacked around it in our code:
so I'm not going to sink more work into it at this time. |
Perhaps we can implement it as |
The
join!
macro forks and joins many expressions at once. This is easier to use than deeply nesting calls torayon::join
.Fixes #402
Note that I wasn't able to create a macro that could be used like:Because there is no way to create gensyms for the temporaries that would help go from the deeply nesting tuples resulting from the nestingrayon::join
calls to a single flat tuple. Rust ends up using the same variable for every tuple element in that case.Second note: I couldn't figure out how to allowlet <irrefutable pattern> = fork ...
, so it only allowsident
s there.Perhaps someone with better macro fu can resolve these issues.r? @cuviper