Hi
On Wed, Aug 30, 2006 at 02:28:57PM +0200, Michel Bardiaux wrote:
> Michael Niedermayer wrote:
> >Hi
> >
> >On Wed, Aug 30, 2006 at 02:00:38PM +0200, Michel Bardiaux wrote:
> >>Michael Niedermayer wrote:
> >>>Hi
> >>>
> >>>On Wed, Aug 30, 2006 at 12:38:30PM +0200, Michael Niedermayer wrote:
> >>>[...]
> >>>>>>>+static void free_catalog(int last_index, char*** pindex_vector)
> >>>>>>>+{
> >>>>>>>+ int i;
> >>>>>>>+ char** index_vector = *pindex_vector;
> >>>>>>>+ for(i=0;index_vector&&i<=last_index;i++)
> >>>>>>>+ av_free(index_vector[i]);
> >>>>>>>+ av_free(index_vector);
> >>>>>>>+ *pindex_vector = NULL;
> >>>>>>av_freep()
> >>>>>>and the index_vector variable seems unneeded
> >>>>>Wilco, but my code was actually correct. It seems to me you *require*
> >>>>>compact code and are ready to reject a patch simply because you find
> >>>>>the coding style too verbose for your taste!
> >>>>yes, code must be simple, patches must be minimal, no superfluous
> >>>>changes
> >>>the coding rules also say that:
> >>>"Main priority in FFmpeg is simplicity and small code size (=less bugs).
> >>>"
> >>>this has been written by fabrice IIRC and was there since a very long
> >>>time
> >>>
> >>>[...]
> >>I would dispute that a 4-liner instead of a 1-liner using ?: generates
> >>more object code; and it does not seem *simpler* to me. I think you tend
> >>too much towards compactness even at the price of obfuscation. But, OK,
> >>I'll try to respect your wishes.
> >
> >looking at for example:
> >
> >>>+int filename_catalog_test(const char *filename)
> >>>+{
> >>>+ if(!filename)
> >>>+ return (-1);
> >>>+ else if(filename[0]=='@')
> >>>+ return 0;
> >>>+ else
> >>>+ return (-1);
> >
> >versus
> >
> >>return filename && filename[0]=='@';
> >
> >i cant see how the later is more obfuscated, additionally
>> I dont mean all one-liners are obfuscated, only that they quickly become
> so. Esp. since the correct one is
>> return (filename && filename[0]=='@')?0:-1; // justified below.
>> Still reasonable, but the one about wildcards is even bigger. If one has
> to code for multiple levels of conditions, nested ?: become quite
> unreadable. So there is a limit to one-liners, and where it is is a
> matter of taste, isnt it?
sure but my suggestion wouldnt need the ?: and without that the one liner
really seems to be the better solution
>> You dont dispute that the object code size is likely to be the same?
it will likely be similar yes
>> >1. is the NULL check really needed at all?
>> I dont know: there was one in the original filename_number_test() so I
> put one too. Keeping maximum symmetry between all 3 kinds of sequences
> seemed a good strategy for implementation.
yes, and factoring that check out and putting it before the decission which
of the 3 functions is called also seems like its a good idea
>> >2. in C 0 is false not 0 is true, using 0 as true is broken as logical
> >and/or
> > will no longer have their intuitive meaning, do you disagree here?
>> Not at all. I knew you want patches to be as little intrusive as
> possible, so I tried not to change anything to numbered sequences, which
> are known to work. Hence I had to conform to the API defined by
> get_frame_filename and filename_number_test, which is on the lines of
> UNIX-style syscalls. Maybe this has to be changed, but shouldnt that be
> a different patch?
of course it should be a seperate patch :)
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
In the past you could go to a library and read, borrow or copy any book
Today you'd get arrested for mere telling someone where the library is