human-readable: calculate numbers in a loop

[eworm-de]

This drops a lot of else if blocks and extends units by "E", "Z" & "Y".

[iliruci23-hue]

Ismail

[steadytao]

This needs one portability fix before merge. The new loop calls labs(num / mult) but num is int64; on platforms where int64 is not long that can truncate or otherwise compare the wrong value for large inputs. The comparison can avoid that by staying in int64 arithmetic, for example by comparing both positive and negative ranges directly rather than calling labs().

The docs also list Z and Y but with the current int64 input those units are not reachable in practice. If the implementation only reaches E, I would keep the documentation to the reachable units.

[eworm-de]

Updated - here, and in #459 with some additional changes.

[steadytao]

This is much closer but I think there is still one signed-range issue before merge.

pos_num = num * (num < 0 ? -1 : 1) overflows for INT64_MIN since the minimum signed value cannot be represented as a positive int64. The old code avoided that because it converted to double before negating.

Could the scale-selection loop avoid forming an absolute signed value? For example, compare num / mult against both positive and negative powi thresholds directly, or otherwise handle the minimum signed value without negating it.

[eworm-de]

Ah, yes... Unlikely to hit this 😁️ - but you are right.

But it should be fine when using an unsinged data type? Switched to uint64_t.

I would like to keep the variable, as a follow-up commit (currently in 61802ac) re-uses it.

[steadytao]

Agreed that this is unlikely to be hit in normal use but this helper is specifically in the numeric formatting path and the fix is small enough that I think we should keep it defined for the full int64 range.

There is still one signed-range issue here: assigning to uint64_t does not avoid the overflow because num * -1 is evaluated in signed int64 before conversion. For INT64_MIN, that is still undefined behaviour.

This needs to convert before negating or avoid forming a signed absolute value. For example: uint64_t abs_num = num < 0 ? 0 - (uint64_t)num : (uint64_t)num;

That keeps the INT64_MIN case defined in unsigned arithmetic.

[eworm-de]

I did not want to say that the case is irrelevant, we should definitely get this right. I just wanted to explain why my mind did not came up with this. 😉️

Same for the overflow in calculation - I was not aware of this.

Updated again, I hope it is ok now. Thanks a lot for the detailed review!

[steadytao]

Thank you very much, this looks good to me now. Nice work on cleaning this up and sticking with my tendency for correctness 🤣

[steadytao]

@tridge Could you review and merge when available, cheers mate!

[steadytao]

@eworm-de I will also have a more thorough look at 459 within the next 24-48 hours, thank you for your work thus far.

[eworm-de]

Thanks a lot, and you are welcome!

Let me know if you want #459 to be split into smaller pull requests for review.

[steadytao]

Let me know if you want #459 to be split into smaller pull requests for review.

It's small enough, that'll be fine.