Refactor variadics #638

kkysen · 2022-08-23T06:00:16Z

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 actual lets, 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.

…f` guards.

…n the `args.as_slice()` itself.

…_prefix("__builtin_va_")?`.

…'t have to pass the same args to all of them.

…so many times.

…ons from `match_or!` using `?` and clear `let`s instead. This also allows inlay type hints to work now.

c2rust-transpile/src/translator/variadic.rs

fw-immunant

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`.

thedataking

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.

kkysen added 11 commits August 22, 2022 22:22

Refactor is_va_decl to use .map() and .unwrap_or_default().

3866a89

Refactored match_vacopy with ?.

593da11

Replaced an as &str with a .as_str().

67deb33

Refactored if ... { return None; }s in match arms into match `i…

0560612

…f` guards.

Replaced args.len() == n and then args[0], etc. with matching o…

c2e1136

…n the `args.as_slice()` itself.

Reformatted the match_vapart match after refactoring it.

f9eec5f

Refactored out repeated matching on __builtin_va_* into a `.strip…

51b3d91

…_prefix("__builtin_va_")?`.

Improved doc comments on inner variadic matching functions.

15575c3

Made the variadic-matching inner functions closures instead so we don…

35701ce

…'t have to pass the same args to all of them.

Renamed the local ast_context to ast for brevity since it's used …

578dc62

…so many times.

Removed non-local control flow (return None) and variable declarati…

66cebb5

…ons from `match_or!` using `?` and clear `let`s instead. This also allows inlay type hints to work now.

kkysen requested review from thedataking and fw-immunant August 23, 2022 06:00