Re-implement case set as a macro #1293
Conversation
|
Still need to address some comments from #1292 |
Let me know when this is ready to review. |
|
Ready. Performance is neutral, unfortunately this whole thing didn't make anything faster even though the micro-benchmarks showed improvement. We only need to land this if it is an improvement to readability (which I think it is, personally). |
src/ctx.rs
Outdated
| buf.fill(val) | ||
| /// [`DisjointMut`]: crate::src::disjoint_mut::DisjointMut | ||
| macro_rules! case_set { | ||
| ($UP_TO:literal, $(@DEFAULT=$WITH_DEFAULT:literal,)? $ctx:ident=[$($ctx_expr:expr),* $(,)?], $len:ident=[$($len_expr:expr),* $(,)?], $offset:ident=[$($offset_expr:expr),* $(,)?], $body:block) => { |
There was a problem hiding this comment.
What's the @ in @DEFAULT for? And can we just name it WITH_DEFAULT?
There was a problem hiding this comment.
The @ distinguishes it from $ctx:ident (@ is not a valid identifier, or anything else in prefix position for that matter). Users should not use this version.
There was a problem hiding this comment.
What I really wanted was a way to pass in the boolean to unify the implementations but have two different macros for this (case_set! and case_set_with_default! or whatever).
There was a problem hiding this comment.
It might work if you put it at the end of the args? Especially if len and offset are not caller named. This way is okay, too, though, just funny looking I guess.
| let $len = $len_expr; | ||
| let $offset = $offset_expr; |
There was a problem hiding this comment.
Do these need to be variable names set by the caller? I don't think they're accessed in any $body:block.
There was a problem hiding this comment.
Then we don't have to worry about name collisions like offset and case_offset.
There was a problem hiding this comment.
Only $ctx is accessed by the caller IIUC.
There was a problem hiding this comment.
I see this now, good point. I figured somewhere we access it somewhere from looking at the original C macro, but I guess we don't. It's a bit confusing that the body can access the context variable but not the others. I think I'd prefer to keep it consistent even if we don't use it right now.
I guess we could use let ctx = ... instead to indicate that it creates a new variable name, if we really want to make them different.
There was a problem hiding this comment.
In the old API, though, ctx was explicitly the only extra variable passed to the closure. I don't think it's confusing that the others aren't accessible. ctx is special in this way. It's more confusing to me that there's all these other variables the caller defines.
rinon
force-pushed
the
sjc/case_set_macro
branch
3 times, most recently
from
53d3c2a
to
0b8324b
Compare
rinon
force-pushed
the
sjc/case_set_macro
branch
from
0b8324b
to
e15dbe0
Compare
No description provided.