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?
You dont dispute that the object code size is likely to be the same?
> 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.
> 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?
Greetings,
--
Michel Bardiaux
R&D Director
T +32 [0] 2 790 29 41
F +32 [0] 2 790 29 02
E mailto:mbardiaux at mediaxim.be
Mediaxim NV/SA
Vorstlaan 191 Boulevard du Souverain
Brussel 1160 Bruxelles
http://www.mediaxim.com/