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


--- Comment #0 from Igor Lesik <curoles@yahoo.com> 2010-02-09 03:24:26 PST ---
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: -------

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



--- Comment #1 from Igor Lesik <curoles@yahoo.com> 2010-02-09 18:50:30 PST ---
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: -------

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


Andrei Alexandrescu <andrei@metalanguage.com> changed:

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


--- Comment #2 from Andrei Alexandrescu <andrei@metalanguage.com> 2010-02-10 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: -------

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



--- Comment #3 from Igor Lesik <curoles@yahoo.com> 2010-02-10 23:27:13 PST ---
>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: -------

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: -------

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


Shin Fujishiro <rsinfu@gmail.com> changed:

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


--- Comment #4 from Shin Fujishiro <rsinfu@gmail.com> 2010-05-26 02:59:52 PDT ---
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: -------