Sign conversion may change value in remove_trailing_zeros_traits (implicit conversions)

[djallen89]

In include/dragonbox/dragonbox.h, in each of the remove_trailing_zeros_traits structs, on lines 2974 and 3012, the statement exponent += s; invokes a warning from gcc when -Wconversions and Wsign-conversion are enabled.

I think a reasonable fix is the following:

                assert(s < detail::stdr::numeric_limits<DecimalExponentType>::max());
                auto sAddend = static_cast<DecimalExponentType>(s);
                assert(detail::stdr::numeric_limits<DecimalExponentType>::max() - sAddend > exponent);
                exponent += sAddend;

The suggestion would be better imo if detail::stdr had its own analogue of std::remove_reference, so that decltype could be used properly on exponent.

If s + exponent is expected to be able to wrap, that invokes undefined behavior without the option -fwrapv. If it is intended to wrap, a modification to the algorithm involving an unsigned exponent representation with memcpy's back and forth may be required.

[jk-jeon]

Thanks for raising the issue! In short, overflow can't happen so there is no UB.

In your suggestion,

• The first line is pointless. It's entirely obvious that s cannot be larger than 127.
• std::size_t is probably not the best type for s from the beginning: it should be the type allowing the best codegen, whatever that is. If the choice of type doesn't really matter in that regard, then we could just choose s to be DecimalExponentType so there is no warning. Otherwise, static_cast should be enough.
• Given the correct static guarantee, the second assert is not necessary too but I like it to be there. I found that I didn't take account of trailing zero removal for static range checking of DecimalExponentType though. I will fix it.

[djallen89]

My apologies, I didn't do a more in depth analysis of the code before submitting the issue. Thanks for responding so quickly!

[jk-jeon]

Oh, don't get me wrong. My point was that I will investigate further and come up with a better solution if any. Thanks again!