[MPlayer-cvslog] r23104 - trunk/libass/ass_render.c

[Reimar Döffinger]

Hello,
On Tue, Apr 24, 2007 at 10:51:55PM +0400, Evgeniy Stepanov wrote:
> On Tuesday 24 April 2007 22:26, Ismail Dönmez wrote:
> > On Tuesday 24 April 2007 21:21:43 Reimar Döffinger wrote:
> > > If only info->outline_glyph == 0 is the problem, why didn't you put the
> > > memset in the else, IMO that would make it clearer.
> > > And if it is only a problem because FT_Glyph_Copy, you might consider if
> > > it's a good idea to add something like
> > > // HACK: at least in freetype version ... FT_Glyph_Copy does not work as
> > > // specified for info->outline_glyph
> > > Otherwise someone looking at the code in three years will scratch his
> > > head and wonder why you did it that complicated (yes, svn log is there,
> > > but it will be buried very deep...).
> 
> It's not really a hack, it can be qualified as "being safe". If 
> info->outline_glyph == 0, FT_Glyph_Copy will fail with 
> FT_Err_Invalid_Argument, and, by docs, should set *target to 0, which does 
> not happen. I think, that relying on such behaviour and deliberately passing 
> the function what it thinks is an invalid argument is not a good idea.

ok, that changes things a bit, I misunderstood your log message that NULL is
supposed to be a normal and valid arguments.

> memset could be moved to "else" clause, but I don't find it much cleaner. The 
> logic here is like this: zero-fill the whole struct (always a good thing to 
> do), then assign some of its fields required values.

Well, I thought of all of this as a hack and it is always good to keep a
hack as confined as possible. Since that's not quite the case I agree
with your opinion.

Greetings,
Reimar Döffinger