[FFmpeg-devel] [PATCH] Get rid of p

[Ramiro Polla]

Michael Niedermayer wrote:
> On Sun, Jun 08, 2008 at 08:57:21PM +0100, Ramiro Polla wrote:
>> Michael Niedermayer wrote:
>>> On Fri, Jun 06, 2008 at 05:09:27PM +0100, Ramiro Polla wrote:
>>>> Hello,
>>>>
>>>> By the message subject you probably asked yourself "what 'p'?".
>>>>
>>>> Exactly, I ask myself the same question when reading the source.
>>>>
>>>> I find one-letter variable names hard to grep for and hard to understand. 
>>>> Are patches to get rid of them or use more descriptive names welcome?
>>>>
>>>> Particular cases that come to my mind are:
>>>>  - 's', used throughout FFmpeg to mean context. I suggest to change them to 
>>>> ctx;
>>>>  - 'p', used in video encoders for something that I still have no idea what 
>>>> is doing there.
>>>>
>>>> Ramiro Polla
>>>> Index: utils.c
>>>> ===================================================================
>>>> --- utils.c	(revision 13671)
>>>> +++ utils.c	(working copy)
>>>> @@ -2779,7 +2779,6 @@
>>>>  
>>>>  int64_t parse_date(const char *datestr, int duration)
>>>>  {
>>>> -    const char *p;
>>>>      int64_t t;
>>>>      struct tm dt;
>>>>      int i;
>>>> @@ -2808,12 +2807,11 @@
>>>>  
>>>>      memset(&dt, 0, sizeof(dt));
>>>>  
>>>> -    p = datestr;
>>> int64_t parse_date(const char *p, int duration)
>>>
>>> and your patch is much smaller
>> Hmmm... That goes against the whole purpose of my patch =)
> 
> The purpose being to annoy me? :)
> 
> It just factorizes the change into the unneeded variable copying removial
> and the cosmetic renaming of datestr->p (which i probably wont accept)

The purpose: renaming of datestr->p (which you probably wont accept)

I'm too lazy to write a patch to remove p = datestr; atm =). Might do it 
some other time if someone doesn't beat me to it...

>>> we can discuss the renaming then seperately ...
>>> iam not sure if its good or not for the functions in your patch ...
>> I know my argumentation skills are close to NULL, but I'll try anyways...
>>
>> Once you know what the function does, having small variable names might 
>> be better, but if you're reading it for the first time to try and find 
>> out what it does, having single-letter or non-descriptive names makes it 
>> harder to understand. Or if you're debugging and stumble upon those 
>> variables, it takes longer to find out what they actually are.
>>
>> I use FFmpeg's source code as a basis to learn lots of things. I prefer 
>> learning from well-written C code than textual descriptions like 
>> textbooks or tutorials, and I'm probably not the only one.
>>
> 
>> Another reason is that searching for any of occurrence of some variable 
>> becomes hard, since its name is part of many other variable names and 
>> mostly anything.
> 
> get cscope

Interesting...

>> I found this specially painful when reading mpeg code for the mimic 
>> encoder. Long functions with lots of s around...
> 
> would it really have helped if these where lots of ctx instead? If so
> iam curious how?

Ctrl+F ctx F3 F3 F3...
Or / ctx n n n... if you prefer

[^a-z]s[^a-z] expects the search function of whatever editor to support 
regexp. Maybe it's just because I grew out of Windows, but regexps are 
much less intuitive for me, even if they are more powerful, specially 
for simple searches.

Ramiro Polla