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

transpile: simplify dynamic argument parsing #538

Merged
merged 1 commit into from
Jul 18, 2022

Conversation

afetisov
Copy link
Contributor

Instead of it.next().unwrap().parse().unwrap() chains use slice patterns and a single expect call.

Copy link
Contributor

@kkysen kkysen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left some small suggestions on error messages and naming, but the main issue here is that previously bubble-up errors are now panicked, i.e. .ok_or(...)? was changed to .expect(...). I added suggestions to change these back to .map_err(|_| ...)? to get the previous behavior back. In the simd.rs case, .expect could be okay as the array is created statically and has exactly 3 elements, but .map_err(|_| ...)? is still better to keep consistency with the old behavior, and avoid introducing panics when refactoring later. Once it's changed to .map_err(|_| ...)?, this should be good to go.

c2rust-transpile/src/translator/simd.rs Outdated Show resolved Hide resolved
c2rust-transpile/src/translator/simd.rs Show resolved Hide resolved
c2rust-transpile/src/translator/builtins.rs Outdated Show resolved Hide resolved
c2rust-transpile/src/translator/simd.rs Outdated Show resolved Hide resolved
c2rust-transpile/src/translator/builtins.rs Outdated Show resolved Hide resolved
c2rust-transpile/src/translator/builtins.rs Outdated Show resolved Hide resolved
Instead of `it.next().unwrap().parse().unwrap()` chains use slice patterns and a single `expect` call.
@kkysen kkysen merged commit 763e230 into immunant:master Jul 18, 2022
@afetisov afetisov deleted the dynamic-args branch July 18, 2022 22:47
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.

None yet

2 participants