From: Kyle J. McKay <mackyle@gmail.com>
Date: Thu, 22 May 2014 23:17:59 GMT
Subject: Re: [PATCH 2/2] strbuf: add strbuf_tolower function
Message-ID: <4831E838-1373-42F6-B490-2D2943E319C9@gmail.com>
In-Reply-To: <20140522134253.GA16408@sigill.intra.peff.net>

On May 22, 2014, at 06:42, Jeff King wrote:

> [re-adding list cc]
>
> On Thu, May 22, 2014 at 03:16:45PM +0200, Christian Couder wrote:
>
>>> +void strbuf_tolower(struct strbuf *sb)
>>> +{
>>> +       char *p;
>>> +       for (p = sb->buf; *p; p++)
>>> +               *p = tolower(*p);
>>> +}
>>
>> Last time I tried a change like the above, I was told that strbufs  
>> are
>> buffers that can contain NUL bytes. So maybe it should be:
>>
>>       for (p = sb->buf; p < sb->buf + sb->len; p++)
>>              *p = tolower(*p);
>
> Hah. I wrote it like that originally, and in review was asked to  
> switch
> it. I agree that it might be worth keeping strbuf functions 8bit- 
> clean.
> The original intent of the strbuf code was to make C strings easier to
> use, but I think we sometimes use them as general buffers, too.


I didn't see this patch before I sent the other email, but this is the  
relevant part:

On May 22, 2014, at 15:52, Kyle J. McKay wrote:
> The only reason I brought up the code difference in the first place  
> was that the comment was "This makes config's lowercase() function  
> public" which made me expect to see basically the equivalent of  
> replacing a "static" with an "extern", but actually the function  
> being made public was a different implementation than config's  
> lowercase() function.  So it looks like the original PATCH 2/9  
> version of the code is best, but perhaps the comment can be tweaked  
> a bit to not convey the same impression I got.  Maybe something like  
> "Replace config's lowercase() function with a public one".


Or even just delete the "This makes config's lowercase() function  
public" line after switching the implementation back.

--Kyle