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

           Summary: isNumeric causes a stack overlfow with mutable arrays
           Product: D
           Version: 2.036
          Platform: Other
        OS/Version: Windows
            Status: NEW
          Severity: normal
          Priority: P2
         Component: Phobos
        AssignedTo: nobody@puremagic.com
        ReportedBy: sandford@jhu.edu


--- Comment #0 from Rob Jacques <sandford@jhu.edu> 2009-12-11 14:04:54 PST ---
Test case:

import std.string;

void main(string[] args) {
    isNumeric("1".dup);
}

This results in a stack overflow on DMD 2.037.
Note that immutable arrays work correctly. (i.e. isNumeric("1".idup); is okay)

-- 
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=3609


David Simcha <dsimcha@yahoo.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |dsimcha@yahoo.com


--- Comment #1 from David Simcha <dsimcha@yahoo.com> 2009-12-11 16:31:10 PST ---
I ran into this a few days, too and was meaning to file it.  isNumeric is **completely** busted on D2, and it amazes me that noone noticed it until now. The problem is that isNumeric(string, bool) is overloaded against isNumeric(...).  In D1, string is an alias for char[].  In D2, it's an alias for immutable(char)[].  If the type that's input is not a string, it's sent to isNumeric(...).  If it's a mutable char[] array in D2, the following is executed:

    if (_arguments[0] == typeid(char[]))
        return isNumeric(va_arg!(char[])(_argptr));

Obviously, the intent here is to call the overload, but in D2 the overload
expects an immutable(char)[], leading to isNumeric(...) getting called again.
Violia, infinite recursion and stack overflows.

I doubt anyone actually uses isNumeric(...), because its functionality should really be handled at compile time w/ templates.  I doubt there are too many use cases for having it be handled at runtime.  Therefore, I recommend dumping it and templating isNumeric(string) to handle char[], wchar[], dchar[], and the const and immutable flavors of these.  This would also fix bug 3610.

-- 
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=3609


Andrei Alexandrescu <andrei@metalanguage.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |andrei@metalanguage.com


--- Comment #2 from Andrei Alexandrescu <andrei@metalanguage.com> 2009-12-11 21:24:55 PST ---
(In reply to comment #1)
> I ran into this a few days, too and was meaning to file it.  isNumeric is **completely** busted on D2, and it amazes me that noone noticed it until now. The problem is that isNumeric(string, bool) is overloaded against isNumeric(...).  In D1, string is an alias for char[].  In D2, it's an alias for immutable(char)[].  If the type that's input is not a string, it's sent to isNumeric(...).  If it's a mutable char[] array in D2, the following is executed:
> 
>     if (_arguments[0] == typeid(char[]))
>         return isNumeric(va_arg!(char[])(_argptr));
> 
> Obviously, the intent here is to call the overload, but in D2 the overload
> expects an immutable(char)[], leading to isNumeric(...) getting called again.
> Violia, infinite recursion and stack overflows.
> 
> I doubt anyone actually uses isNumeric(...), because its functionality should really be handled at compile time w/ templates.  I doubt there are too many use cases for having it be handled at runtime.  Therefore, I recommend dumping it and templating isNumeric(string) to handle char[], wchar[], dchar[], and the const and immutable flavors of these.  This would also fix bug 3610.

IMHO it's kind of silly to define isNumeric. It does about the same amount of work as to, but doesn't give you the actual number. I suggest we just yank it.

-- 
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=3609



--- Comment #3 from David Simcha <dsimcha@yahoo.com> 2009-12-11 21:43:25 PST ---
(In reply to comment #2)
> IMHO it's kind of silly to define isNumeric. It does about the same amount of work as to, but doesn't give you the actual number. I suggest we just yank it.

The only problem I see with this is that then you're using exception handling as flow control if your goal was just to validate something.  Then again, this might not be a big deal because:

1.  Why would you be validating something as being numeric if you have no intention of converting it to a number?

2.  One reason why using exceptions as flow control is considered bad is performance.  Not too many people try to convert strings to numbers inside performance critical code.

3.  The other reason why exceptions as flow control is considered bad is that it's unstructured flow control, kind of like goto.  However, in this case I don't think that applies.

real foo;
try {
    foo = to!real(someString);
} catch(ConvError) {
    // yada yada yada
}

Seems pretty clear and readable to me.

IMHO taking it out is probably a good move but I think we should think twice and allow others to comment b/c it does have the downside of encouraging using exceptions as flow control.

-- 
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=3609


Andrei Alexandrescu <andrei@metalanguage.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |ASSIGNED
         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=3609


Andrej Mitrovic <andrej.mitrovich@gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |RESOLVED
                 CC|                            |andrej.mitrovich@gmail.com
         Resolution|                            |FIXED


--- Comment #4 from Andrej Mitrovic <andrej.mitrovich@gmail.com> 2011-05-26 13:41:41 PDT ---
I can see from the year 2009 checkout that isNumeric took a string, but now it takes a const(char)[] s, and this seems to fix the issue. I'm not having stack overflows here. I'll mark this as fixed.

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