ulfjack / ryu

Converts floating point numbers to decimal strings
Apache License 2.0
1.2k stars 100 forks source link

Is this dead branch a bug? #163

Closed forksnd closed 4 years ago

forksnd commented 4 years ago

The branch on this line seems to be dead since nonzero is explicitly declared to be false and then is never modified until later. Since there is no corresponding else branch (only an else if), this part of the code smells a bit fishy.

ulfjack commented 4 years ago

The loop behaves differently once it sees the first nonzero digit. That is perfectly intentional. Maybe the variable should be called hasSeenNonZeroDigits or something like that. Let me give an example - the digits the loop computes look like this: 0000000000000000000123456789000000000000012345000000000

We want to print: 123456789000000000000012345000000000

I.e., we want to eliminate leading zeroes. We could use two loops here, but they would be almost identical. Instead, we use a flag to indicate that we have seen the first non-zero digit. Note that there may be a section of all zeroes afterwards, so it's not enough to just look at whether the digits are non-zero.

Btw. the else case is to skip digits. I.e., the code works like this:

if (seen_non_zero_digit) {
  print9digits();
} else if (digits != 0) {
  print_some_number_of_digits();
  seen_non_zero_digit = true;
} else {
  // seen_non_zero_digit == false
  // digits == 0
  skip();
}

Makes sense?

forksnd commented 4 years ago

Ah, I completely misunderstood the logic. Thanks for the explaination.

You might be interested to know that I have been porting Ryu to my printf implementation (see print.h, ryu.h and ryu_tables.h. Currently, it's more or less a strategic copy-paste job with modifications to fit the existing architecture (and some printf-related amenities added), but I do intend to slowly rewrite the code to make it more understandable.

I did have a few more questions, if you don't mind:

  1. What is the semantic different between append_n_digits, append_d_digits and append_c_digits (besides the obvious difference in logic)? For example, is there a particular reason why append_d_digits is also responsible for copying the period, while the others are not?

  2. Is using a smaller table for the fixed representation on the roadmap? Currently, the tables alone take up > 102 KiB of space in the final executable.

Thank you.

ulfjack commented 4 years ago

The %e format requires exactly one digit before the decimal point, and we decided to have a separate function for that (append_d_digits) because the algorithm generates the digits as a single integer; I am not aware of a fast way of implementing it in terms of one of the other methods. Note in particular that we must avoid division by a non-constant, because those are sloooooooow. It's hard to exaggerate how slow they are - at one point I fixed such a case and the entire code got 2x faster.

ulfjack commented 4 years ago

There's a small semantic difference between append_n_digits and append_c_digits, but I'm not sure right now if it's necessary to have two separate functions there.