Bug 5232 – [patch] std.conv.to & std.conv.roundTo report invalid overflows for very large numbers

This comes from using roundTo!ulong with a real equal to ulong.max. Here's the unit test: real r = ulong.max; assert( (cast(ulong)r) == ulong.max , "Okay"); assert( to!ulong(r) == ulong.max , "Okay"); assert( roundTo!ulong(r) == ulong.max , "Conversion overflow"); The reason for this is that toImpl uses casting, which implies truncation, but tests for overflow by simple comparison and roundTo adds 0.5 to the source value. Here's a patch for toImpl: T toImpl(T, S)(S value) if (!implicitlyConverts!(S, T) && std.traits.isNumeric!(S) && std.traits.isNumeric!(T)) { enum sSmallest = mostNegative!(S); enum tSmallest = mostNegative!(T); static if (sSmallest < 0) { // possible underflow converting from a signed static if (tSmallest == 0) { immutable good = value >= 0; } else { static assert(tSmallest < 0); immutable good = value >= tSmallest; } if (!good) ConvOverflowError.raise("Conversion negative overflow"); } static if (S.max > T.max) { // possible overflow - if (value > T.max) ConvOverflowError.raise("Conversion overflow"); + if (value >= T.max+1.0L) ConvOverflowError.raise("Conversion overflow"); } return cast(T) value; } As a note, the roundTo unit test still fails because reals can't represent ulong.max+0.5 and thus the implementation of roundTo effectively adds 1.0 instead of 0.5 (And thus it is still broken) Also, it should probably use template constraints instead of static asserts. Here's a patch for both issues: template roundTo(Target) { Target roundTo(Source)(Source value) if( isFloatingPoint!Source && isIntegral!Target ) { return to!(Target)( round(value) ); } }

Minor patch update. Updated the template constraints to DMD 2.052. Changed +1.0L to +1.0uL and shifted the formating to a 80-character maximum line length. As before this patch is essentially a single line change from if (value > T.max) to if (value >= T.max+1.0L) /** Narrowing numeric-numeric conversions throw when the value does not fit in the narrower type. */ T toImpl(T, S)(S value) if (!implicitlyConverts!(S, T) && (isNumeric!S || isSomeChar!S) && (isNumeric!T || isSomeChar!T)) { enum sSmallest = mostNegative!(S); enum tSmallest = mostNegative!(T); static if (sSmallest < 0) { // possible underflow converting from a signed static if (tSmallest == 0) { immutable good = value >= 0; } else { static assert(tSmallest < 0); immutable good = value >= tSmallest; } if (!good) ConvOverflowException.raise("Conversion negative overflow"); } static if (S.max > T.max) { // possible overflow if (value >= T.max+1.0uL) ConvOverflowError.raise("Conversion overflow"); } return cast(T) value; }

Opps. Forgot that 1.0L is a real, not a long. Also, that ConvOverflowError changed to ConvOverflowException. /** Narrowing numeric-numeric conversions throw when the value does not fit in the narrower type. */ T toImpl(T, S)(S value) if (!implicitlyConverts!(S, T) && (isNumeric!S || isSomeChar!S) && (isNumeric!T || isSomeChar!T)) { enum sSmallest = mostNegative!(S); enum tSmallest = mostNegative!(T); static if (sSmallest < 0) { // possible underflow converting from a signed static if (tSmallest == 0) { immutable good = value >= 0; } else { static assert(tSmallest < 0); immutable good = value >= tSmallest; } if (!good) ConvOverflowException.raise("Conversion negative overflow"); } static if (S.max > T.max) { // possible overflow if (value >= T.max+1.0L) ConvOverflowException.raise("Conversion overflow"); } return cast(T) value; }

The proposed solution doesn't work, because x87er reals cannot save values between ulong.max and ulong.max + 1. And for real==double this should also not work.

@berni44 created dlang/phobos pull request #7334 "Fix Issue 5232 - [patch] std.conv.to & std.conv.roundTo report invalid overflows for very large numbers" fixing this issue: - Fix Issue 5232 - [patch] std.conv.to & std.conv.roundTo report invalid overflows for very large numbers https://github.com/dlang/phobos/pull/7334

dlang/phobos pull request #7334 "Fix Issue 5232 - [patch] std.conv.to & std.conv.roundTo report invalid overflows for very large numbers" was merged into master: - 6a1b88fcb8a9b78665c1d2b3a94cd336957d3fc5 by Bernhard Seckinger: Fix Issue 5232 - [patch] std.conv.to & std.conv.roundTo report invalid overflows for very large numbers https://github.com/dlang/phobos/pull/7334