[MPlayer-cvslog] r33059 - trunk/gui/mplayer/gui_common.c

[Clément Bœsch]

On Tue, Mar 08, 2011 at 10:27:29PM +0100, Ingo Brückl wrote:
> Clément Boesch wrote on Tue, 8 Mar 2011 22:08:12 +0100:
> 
> > Btw, what's the point of having trbuf static if it was zero'ed?
> 
> I don't get it.
> 
> It is the return value, so it has to be static, and it is av_strlcat'ed so it
> must be erased - but not the whole buffer. (The function is called repeatedly
> to update the dynamic labels.)
> 

I didn't look at the function and so missed that part. This is bad
practice thought; the calling function should be responsible for the
buffer allocation (stack or not), or Translate should dynamically allocate
one.

Btw, note that it seems this function can be made static.

> >> 3. The array indices are unsigned now, and the manual optimization of
> >>    having strlen() outside the for loop has been removed in favor of
> >>    optimization performed by the compiler.
> >>
> 
> > I'm not convinced this will ever be optimized. Also, it could mean, from a
> > developer point of view, that the length could change during the loop. The
> > function being pretty big, it's hard to get sure if it is or not.
> 
> That's up to the compiler. ;-)
> 

Up to the compiler to explain the code to the developer? :P

> But seriously, there are so many awkward strlen() (even to check for empty
> strings) that I've put their revision on my list. Most of them will go some
> time. A string can be run through by a char pointer and a check for 'length
> is 0' doesn't need this call either.
> 

Not sure to get your point. I was just stating that it's not that clear
for the developer to know whether the call is needed or not. But, again,
I'm not asking you to change anything, just noticing you.

-- 
Clément B.