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);
}
Comment #1 by curoles — 2010-02-09T18:50:30Z
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);
}
Comment #2 by andrei — 2010-02-10T22:35:01Z
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.
Comment #3 by curoles — 2010-02-10T23:27:13Z
>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).