Re: [AGGREGATED PATCH] Fix in-place editing functions in convert.c

[Pierre Habouzit <madcoder@debian.org>]

[-- Attachment #1: Type: text/plain, Size: 1820 bytes --]

On Fri, Oct 05, 2007 at 04:21:39PM +0000, Sam Ravnborg wrote:
> On Fri, Oct 05, 2007 at 08:26:44AM -0700, Linus Torvalds wrote:
> > 
> > 
> > On Fri, 5 Oct 2007, Pierre Habouzit wrote:
> > >  
> > > -	strbuf_grow(buf, len);
> > > +	/* only grow if not in place */
> > > +	if (strbuf_avail(buf) + buf->len < len)
> > > +		strbuf_grow(buf, len - buf->len);
> > 
> > Umm. This is really ugly.
> > 
> > The whole point of strbuf's was that you shouldn't be doing your own 
> > allocation decisions etc. So why do it?
> > 
> > Wouldn't it be much better to have a strbuf_make_room() interface that 
> > just guarantees that there is enough room fo "len"? 
> > 
> > Otherwise, code like the above would seem to make the whole point of a 
> > safer string interface rather pointless. The above code only makes sense 
> > if you know how the strbuf's are internally done, so it should not exists 
> > except as internal strbuf code. No?
> 
> Took a short look at strbuf.h after seeing the above code.
> And I was suprised to see that all strbuf users were exposed to
> the strbuf structure.
> Following patch would at least make sure noone fiddle with strbuf internals.
> Cut'n'paste - only for the example of it.
> It simply moves strbuf declaration to the .c file where it rightfully belongs.

  you're looking at an antiquated version, please look at the one in
current master on current next. In this one, what you can do or not do
with the struct is explained

> git did not build with this change....

  Of course it doesn't! people want to have direct access to ->buf and
->len, and it's definitely OK.

-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]