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

Handle exponents in FixedDecimal::from_str() #1265

Merged
merged 4 commits into from
Nov 5, 2021

Conversation

Manishearth
Copy link
Member

Needed for #166

zbraniecki
zbraniecki previously approved these changes Nov 5, 2021
Copy link
Member

@zbraniecki zbraniecki left a comment

Choose a reason for hiding this comment

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

logic looks good and the code is readable. I think Rust offers a number of nice APIs for iterating that would make it even cleaner, but nothing blocking, so up to Manish to include or reject my suggestions.

let mut dot_index = no_sign_str_len;
let mut has_exponent = false;
let mut dot_index = no_sign_str.len();
let mut exponent_index = no_sign_str.len();
Copy link
Member

Choose a reason for hiding this comment

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

question: would Option(index) be a better encapsulation for index and exponent?

Copy link
Member Author

Choose a reason for hiding this comment

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

We still use exponent_index even if it's an option, though. I can rename that to exponent_or_end_index

Copy link
Member

Choose a reason for hiding this comment

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

Isn't expontent.is_none() equivalent of no_sign_str.len() then?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I mean that every time we use exponent_index we want to use exponent.unwrap_or(no_sign_str.len()) anyway, so it's not worth it to store the Option

}
dot_index = i;
has_dot = true;
if i == 0 || i == no_sign_str.len() - 1 {
Copy link
Member

Choose a reason for hiding this comment

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

question: it seems like you could get rid of no_sign_str if you just checked the dot: Option(index) after the loop for if it is set to the length of the string.

if has_exponent && exponent_index == i - 1 {
continue;
} else {
return Err(Error::Syntax);
}
} else if *c < b'0' || *c > b'9' {
Copy link
Member

Choose a reason for hiding this comment

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

question: is that faster than is_ascii_digit?

Copy link
Member Author

Choose a reason for hiding this comment

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

We don't have a char

Copy link
Member

Choose a reason for hiding this comment

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

utils/fixed_decimal/src/decimal.rs Outdated Show resolved Hide resolved
utils/fixed_decimal/src/decimal.rs Outdated Show resolved Hide resolved
@@ -570,7 +598,7 @@ impl FromStr for FixedDecimal {

// Constructing DecimalFixed.digits
let mut v: SmallVec<[u8; 8]> = SmallVec::with_capacity(digits_str_len);
for c in no_sign_str[leftmost_digit..rightmost_digit].iter() {
for c in no_exponent_str[leftmost_digit..rightmost_digit].iter() {
Copy link
Member

Choose a reason for hiding this comment

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

question: filter_map?

if has_exponent {
let mut pow = 0;
let mut pos_neg = 1;
for digit in &no_sign_str[exponent_index + 1..] {
Copy link
Member

Choose a reason for hiding this comment

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

question: reduce?

zbraniecki
zbraniecki previously approved these changes Nov 5, 2021
Copy link
Member

@zbraniecki zbraniecki left a comment

Choose a reason for hiding this comment

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

lgtm! (I still like the Option(index) more, but leave it to the PR author discretion).

@Manishearth
Copy link
Member Author

Yeah overall that code is existing code and I feel like it's just as complex with the Option, with the complexities shifting around

@Manishearth Manishearth merged commit 74e74a3 into unicode-org:main Nov 5, 2021
@Manishearth Manishearth deleted the decimal-scientific branch November 5, 2021 17:12
@sffc
Copy link
Member

sffc commented Nov 6, 2021

I want to discuss scientific notation from an architecture perspective. Filed #1267.

@sffc sffc removed their request for review November 20, 2021 01:31
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.

3 participants