-
Notifications
You must be signed in to change notification settings - Fork 240
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
Refactor variadics #638
base: master
Are you sure you want to change the base?
Refactor variadics #638
Conversation
…n the `args.as_slice()` itself.
…_prefix("__builtin_va_")?`.
…'t have to pass the same args to all of them.
…ons from `match_or!` using `?` and clear `let`s instead. This also allows inlay type hints to work now.
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.
This looks better than the code did before, thanks.
Various requested changes described inline. Be sure to indent continued lines within all match_or!
macro invocations.
…. Now just `*` is used, no "pointer".
…ear that `bool::default() == false`.
… `.strip_prefix("__builtin_va_")?`." It is better to keep these full strings as is for greppability. This reverts commit 51b3d91.
…_vapart`, like already done in `fn match_vastart`.
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.
There are some improvements on this PR but in several places (see comments) I think you should have left things as is vs. making changes that needs reviewing and risks breaking stuff. Please make it a higher priority to avoid code churn going forward.
…2nd line would be preferable, but `rustfmt` says no.
…()`." Avoid code churn that doesn't have clear benefits. This reverts commit 3866a89.
1a5085a
to
2439ed4
Compare
These are various refactors of the variadics code I made when reviewing #612:
Notably, I felt
match_or!
was quite un-idiomatic as it contained hidden non-local control flow (return None
) and defined variables in a non-intuitive way. Thus, I removed these. Using?
makes the control flow not any longer but much clearer, and now we have to use actuallet
s, but this improves readability a lot and allows inlay type hints to work, which are very useful.It might be easier to review by reviewing each commit individually.