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

Idiomatic #23

Merged
merged 25 commits into from Oct 1, 2023
Merged

Idiomatic #23

merged 25 commits into from Oct 1, 2023

Conversation

chris-ha458
Copy link
Contributor

a85c5fe is worth looking at.

This would only be a problem when the file size is around usize::MAX which is around 2^64 bytes.

test passes
- an expensive operation extracted from inside the map.
- mut removed
- Replace `if let Some(encoder) = ...` with `ok_or` for more idiomatic error handling.
- Move error check and return inside the loop for improved flow control.
The previous code would panic in debug and silently wraparound in release.
This change makes it explicitily wraparound in both production and debug.
Other methods are available to make it explicit usize.overflowing_add or usize.checked_add
it isnt clear how it should be handled here though
@chris-ha458
Copy link
Contributor Author

something like this would be idiomatic,
but if 2^64 input is fed in there, I honestly think something would have gone wrong before this point

remaining = remaining.checked_add_signed(err.upto).ok_or_else(|| {
                        CodecError {
                            upto: remaining as isize,
                            cause: Cow::from("Integer overflow occurred"),
                        }
                    })?;

(self.current_ascii_only, character.is_ascii(),
Result of &= operation)

(True	True True)
(True	False	False)
(False	True	False)
(False	False	False)
Every single item is set to default of its type so we can just derive
@nickspring nickspring merged commit 105b91a into nickspring:main Oct 1, 2023
3 checks passed
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