The documentation says
/*******************************************
 * Returns s[] sans trailing delimiter[], if any.
 * If delimiter[] is null, removes trailing CR, LF, or CRLF, if any.
 */

To adhere to the documentation, chomp must be changed from:

C[] chomp(C, C1)(C[] s, in C1[] delimiter)
{
    if (endsWith(s, delimiter))
    {
        return s[0 .. $ - delimiter.length];
    }
    return s;
}

to:

C[] chomp(C, C1)(C[] s, in C1[] delimiter)
{
    if (delimiter == null)
      return chomp(s);
    else if (endsWith(s, delimiter))
      return s[0 .. $ - delimiter.length];
    else
      return s;
}

On Mon, 09 Aug 2010 18:58:36 +0200, simendsjo wrote:

> The documentation says
> /*******************************************
>   * Returns s[] sans trailing delimiter[], if any. * If delimiter[] is
>   null, removes trailing CR, LF, or CRLF, if any. */
> 
> To adhere to the documentation, chomp must be changed from:
> 
> C[] chomp(C, C1)(C[] s, in C1[] delimiter) {
>      if (endsWith(s, delimiter))
>      {
>          return s[0 .. $ - delimiter.length];
>      }
>      return s;
> }
> 
> to:
> 
> C[] chomp(C, C1)(C[] s, in C1[] delimiter) {
>      if (delimiter == null)
>        return chomp(s);
>      else if (endsWith(s, delimiter))
>        return s[0 .. $ - delimiter.length];
>      else
>        return s;
> }


Either that, or the documentation for it needs to be changed.  Anyway, it would be great if you'd report this.

http://d.puremagic.com/issues/

Thanks!

-Lars

Lars T. Kyllingstad:
> Either that, or the documentation for it needs to be changed.  Anyway, it would be great if you'd report this.

A really basic unit testing is able to catch an error like this. This means Phobos needs more unit tests.

Stuff that may be added to that unittest:

import std.stdio, std.string;
void main() {
    assert(chomp("hello") == "hello");
    assert(chomp("hello\n") == "hello");
    assert(chomp("hello\r\n") == "hello");
    assert(chomp("hello", "") == "hello");
    assert(chomp("hello\n", "") == "hello");
    assert(chomp("hello\r\n", "") == "hello");
    assert(chomp("hello\r\n", "") == "hello");
    assert(chomp("helloxxx", "x") == "helloxx");
}


But instead of:
if (delimiter == null)
Is better to write:
if (delimiter.length == 0)
Or:
if (delimiter == "")

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

Bye,
bearophile

On 09.08.2010 19:16, Lars T. Kyllingstad wrote:
> On Mon, 09 Aug 2010 18:58:36 +0200, simendsjo wrote:
>
>> The documentation says
>> /*******************************************
>>    * Returns s[] sans trailing delimiter[], if any. * If delimiter[] is
>>    null, removes trailing CR, LF, or CRLF, if any. */
>>
>> To adhere to the documentation, chomp must be changed from:
>>
>> C[] chomp(C, C1)(C[] s, in C1[] delimiter) {
>>       if (endsWith(s, delimiter))
>>       {
>>           return s[0 .. $ - delimiter.length];
>>       }
>>       return s;
>> }
>>
>> to:
>>
>> C[] chomp(C, C1)(C[] s, in C1[] delimiter) {
>>       if (delimiter == null)
>>         return chomp(s);
>>       else if (endsWith(s, delimiter))
>>         return s[0 .. $ - delimiter.length];
>>       else
>>         return s;
>> }
>
>
> Either that, or the documentation for it needs to be changed.  Anyway, it
> would be great if you'd report this.
>
> http://d.puremagic.com/issues/
>
> Thanks!
>
> -Lars

Ok; http://d.puremagic.com/issues/show_bug.cgi?id=4608

simendsjo:
> Ok; http://d.puremagic.com/issues/show_bug.cgi?id=4608

You seem to have missed my answer :-)

Bye,
bearophile

On 09.08.2010 23:58, bearophile wrote:
> simendsjo:
>> Ok; http://d.puremagic.com/issues/show_bug.cgi?id=4608
>
> You seem to have missed my answer :-)
>
> Bye,
> bearophile

No, but I don't know if it's the documentation or implementation that's correct.

On 09.08.2010 19:34, bearophile wrote:
> Lars T. Kyllingstad:
>> Either that, or the documentation for it needs to be changed.  Anyway, it
>> would be great if you'd report this.
>
> A really basic unit testing is able to catch an error like this. This means Phobos needs more unit tests.
>
> Stuff that may be added to that unittest:
>
> import std.stdio, std.string;
> void main() {
>      assert(chomp("hello") == "hello");
>      assert(chomp("hello\n") == "hello");
>      assert(chomp("hello\r\n") == "hello");
>      assert(chomp("hello", "") == "hello");
>      assert(chomp("hello\n", "") == "hello");
>      assert(chomp("hello\r\n", "") == "hello");
>      assert(chomp("hello\r\n", "") == "hello");
>      assert(chomp("helloxxx", "x") == "helloxx");
> }
>
>
> But instead of:
> if (delimiter == null)
> Is better to write:
> if (delimiter.length == 0)
> Or:
> if (delimiter == "")
>
> See http://d.puremagic.com/issues/show_bug.cgi?id=3889
>
> Bye,
> bearophile

Ahem.. :) Yes, I did miss your answer! How I got fooled by the preview pane and never noticed the scrollbar.
I cannot see how your other bug report relates to this though. For chomps part it's just an implementation vs. documentation issue.

simendsjo:
> Ahem.. :) Yes, I did miss your answer! How I got fooled by the preview pane and never noticed the scrollbar.

No problem, it happens, don't worry.


> I cannot see how your other bug report relates to this though.

My other bug report is about this line in your code:
 if (delimiter == null)
I don't like it :-)

Bye,
bearophile

On Monday, August 09, 2010 16:59:07 bearophile wrote:
> simendsjo:
> > Ahem.. :) Yes, I did miss your answer! How I got fooled by the preview pane and never noticed the scrollbar.
> 
> No problem, it happens, don't worry.
> 
> > I cannot see how your other bug report relates to this though.
> 
> My other bug report is about this line in your code:
>  if (delimiter == null)
> I don't like it :-)
> 
> Bye,
> bearophile

Why, because it should be

if(delimiter is null)


or just

if(!delimiter)


- Jonathan M Davis

On 10.08.2010 02:09, Jonathan M Davis wrote:
> On Monday, August 09, 2010 16:59:07 bearophile wrote:
>> simendsjo:
>>> Ahem.. :) Yes, I did miss your answer! How I got fooled by the preview
>>> pane and never noticed the scrollbar.
>>
>> No problem, it happens, don't worry.
>>
>>> I cannot see how your other bug report relates to this though.
>>
>> My other bug report is about this line in your code:
>>   if (delimiter == null)
>> I don't like it :-)
>>
>> Bye,
>> bearophile
>
> Why, because it should be
>
> if(delimiter is null)
>
>
> or just
>
> if(!delimiter)
>
>
> - Jonathan M Davis

Hehe.. You're a bit beyond my D level right now. At least I now know null == false and you can do reference is null :)