http://d.puremagic.com/issues/show_bug.cgi?id=5232

           Summary: [patch] std.conv.to & std.conv.roundTo report invalid
                    overflows for very large numbers
           Product: D
           Version: D2
          Platform: Other
        OS/Version: Windows
            Status: NEW
          Keywords: patch
          Severity: minor
          Priority: P2
         Component: Phobos
        AssignedTo: nobody@puremagic.com
        ReportedBy: sandford@jhu.edu


--- Comment #0 from Rob Jacques <sandford@jhu.edu> 2010-11-17 22:00:45 PST ---
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) );
    }
}

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

http://d.puremagic.com/issues/show_bug.cgi?id=5232


Andrei Alexandrescu <andrei@metalanguage.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |ASSIGNED
                 CC|                            |andrei@metalanguage.com
         AssignedTo|nobody@puremagic.com        |andrei@metalanguage.com


-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

http://d.puremagic.com/issues/show_bug.cgi?id=5232



--- Comment #1 from Rob Jacques <sandford@jhu.edu> 2011-03-16 09:40:33 PDT ---
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;
}

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

http://d.puremagic.com/issues/show_bug.cgi?id=5232



--- Comment #2 from Rob Jacques <sandford@jhu.edu> 2011-03-16 17:22:28 PDT ---
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;
}

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------