[MPlayer-dev-eng] patch: can't display mp3

[Attila Kinali]

On Tue, 21 Dec 2004 22:28:24 +0900
"Yasuhiro Matsumoto" <mattn_jp at hotmail.com> wrote:

> > > Ok, please check this.
> >
> >--- libmpdemux/demuxer.c.orig	2004-11-26 07:24:00.000000000 +0900
> >+++ libmpdemux/demuxer.c	2004-12-20 09:19:22.000000000 +0900
> >@@ -21,6 +21,15 @@
> >+iconv_string(iconv_t fd, char *str)
> >+{
> >
> >You should add here a documentation entry, see 
> >DOCS/tech/code-documentation.txt
> 
> ok, i added few comments.
> (sorry, i'm not native speaker)

The function itself is still not documented.
Please read DOCS/tech/code-documentation.txt again.
Every function has to be documented, what it is doing,
what its input parameter are, their valid range and
the output parameter.

And while you are at it, please also read DOCS/tech/patches.txt.
You should not compress patches unless it's really needed.
It just adds another step for the one reviewing it.

> 
> >[...]
> >+		if (len == 0 || errno == E2BIG)
> >+		{
> >+			len = len + fromlen * 2 + 40;
> >
> >Where do these values come from ?
> 
> see the comment.

I rather asked why you choose *2+40, ie is there a
special reason to double and add 40 to it ?

Beside, you are allocating more space on every round,
which can make the buffer quite big. You should rather
check whether you need more space or not.

> >+			p = (char*)malloc((unsigned)len);
> >+			if (p != NULL && done > 0)
> >+				memcpy(p, result, done);
> >+			free(result);
> >+			result = p;
> >+			if (result == NULL)
> >+				break;
> >+		}
> >
> >A realloc() would be cleaner here
> 
> hmmm, i don't understand this.
> remalloc(NULL, size) won't work on some environment.
> and, we should write "malloc" and "realloc". and checking fail.
> my part won't leak when fail of malloc().


if(p)
	realloc(...)
else
	malloc(...)

I also dont see how this should leak.



				Attila Kinali

-- 
All christians are terrorists.