Clean up StringBuilder::appendIntegral

XMLWordPrintableJSON

    • Type: Task
    • Resolution: Unresolved
    • Priority: Major - P3
    • None
    • Affects Version/s: None
    • Component/s: None
    • Server Programmability
    • Fully Compatible
    • 200
    • None
    • None
    • None
    • None
    • None
    • None
    • None

      I don't think there was actually a bug, but there were two near-bugs. I wasted time reading the rules for [usual arithmetic conversion](https://en.cppreference.com/w/c/language/conversion.html) again since I thought this was wrong. Hopefully these changes make the code obviously correct so that nobody has to think or rely on those arcane rules again when reading this code because it no longer mixes signed an unsigned numbers in binary ops.

      1. with MIN_INT64: `0 - uint64_t(val)` looks like it might convert both arguments to signed to match 0, which is a problem because it would be signed overflow. Luckily it converts the 0 to uint64_t instead. I made it just use unary negation which is defined as performing the two's-complement transformation on unsigned numbers.

      2. with unsigned T: `val < 0` looks like it would do the wrong thing for numbers greater than max sized int. But do to the strange combination of integral promotion and the usual conversions, the 0 comparison would be performed using the same signedness as `val`. I made it just do `T(0)` so that both sides of the `<` always have the same type and signedness without needing conversions.

      In addition to those, I also converted the is_integral static_assert to a concept check, and added a static_assert that we never pass integers larger than uint64_t since this implementation can't handle them and it seems better to fail compile than silently truncate here. If we ever need to, this could be converted to a `requires` check and we could add a different implementation for larger integers.

            Assignee:
            Mathias Stearn
            Reporter:
            Mathias Stearn
            Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

              Created:
              Updated: