www.digitalmars.com         C & C++   DMDScript  

digitalmars.D.bugs - [Issue 3786] New: bug in std.string.removechars

reply d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=3786

           Summary: bug in std.string.removechars
           Product: D
           Version: 2.039
          Platform: x86
        OS/Version: Windows
            Status: NEW
          Severity: normal
          Priority: P2
         Component: Phobos
        AssignedTo: nobody puremagic.com
        ReportedBy: curoles yahoo.com
                CC: curoles yahoo.com



I do not understand how std.string.removechars could pass the unittest. It does
not work for me and its implementation looks buggy, imho.

    foreach (size_t i, dchar c; s)
    {
        if (inPattern(c, pattern)) continue; <-- here we must check change flag

        if (!changed)
        {   changed = true;
            r = s[0 .. i].dup; <----- Why 0? what if we skipped 1st char?

The problem could be fixed by replacing [0..i] with [i..i], but then the
original idea to optimize some cases does not work at all. Speculating what the
author had in mind writing this code, I would rewrite it this way (passes the
unittest btw):

    foreach (size_t i, dchar c; s)
    {
        if (inPattern(c, pattern)) {
                if (!changed)
                {   changed = true;
                    r = s[0 .. i].dup;
                }
                continue;
        }
        if (changed)
        {
            std.utf.encode(r, c);
        }

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Feb 09 2010
next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=3786




Just in case, the correct version of the function might look like:

string removechars(string s, in string pattern)
{
    char[] r;
    bool changed = false;

    foreach (size_t i, dchar c; s)
    {
        if (inPattern(c, pattern)){
                if (!changed)
                {   changed = true;
                    r = s[0 .. i].dup;
                }
                continue;
        }
        if (changed)
        {
            std.utf.encode(r, c);
        }
    }
    return (changed? r.idup : s);
}

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Feb 09 2010
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=3786


Andrei Alexandrescu <andrei metalanguage.com> changed:

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



22:35:01 PST ---
I think the best way to prove the problems with the current version and the
fixes made by the proposed versions is by writing a few unittests.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Feb 10 2010
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=3786





test3.exe


core.exception.AssertError (22): Assertion failure

16    string r;
17
18    r = MYremovechars("hah", "h");
19    assert(r == "a");
20
21    r = removechars("hah", "h");
22    assert(r == "a");

I hope it proves my point.

The problem with existing unittest for removechars is that
in ALL cases characters to be removed go back-to-back (this is
why removechars passes existing unittest).

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Feb 10 2010
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=3786


Shin Fujishiro <rsinfu gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |ASSIGNED
                 CC|                            |rsinfu gmail.com
         AssignedTo|nobody puremagic.com        |rsinfu gmail.com


-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
May 24 2010
prev sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=3786


Shin Fujishiro <rsinfu gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |RESOLVED
         Resolution|                            |FIXED



---
Fixed in svn r1555. Thanks for the correct code!

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