[FFmpeg-devel] [PATCH] doc/developer: update style guidelines to include for loops with declarations

James Almer jamrial at gmail.com
Thu Nov 9 02:51:43 EET 2017


On 11/8/2017 9:39 PM, Rostislav Pehlivanov wrote:
> On 8 November 2017 at 23:05, Mark Thompson <sw at jkqxz.net> wrote:
> 
>> On 08/11/17 22:41, Rostislav Pehlivanov wrote:
>>> On 8 November 2017 at 22:20, Mark Thompson <sw at jkqxz.net> wrote:
>>>
>>>> On 08/11/17 22:03, Rostislav Pehlivanov wrote:
>>>>> On 8 November 2017 at 21:49, Mark Thompson <sw at jkqxz.net> wrote:
>>>>>
>>>>>> On 08/11/17 21:26, Rostislav Pehlivanov wrote:
>>>>>>> Signed-off-by: Rostislav Pehlivanov <atomnuker at gmail.com>
>>>>>>> ---
>>>>>>>  doc/developer.texi | 3 +++
>>>>>>>  1 file changed, 3 insertions(+)
>>>>>>>
>>>>>>> diff --git a/doc/developer.texi b/doc/developer.texi
>>>>>>> index a7b4f1d737..de7d887451 100644
>>>>>>> --- a/doc/developer.texi
>>>>>>> +++ b/doc/developer.texi
>>>>>>> @@ -132,6 +132,9 @@ designated struct initializers (@samp{struct s x
>> =
>>>>>> @{ .i = 17 @};});
>>>>>>>  @item
>>>>>>>  compound literals (@samp{x = (struct s) @{ 17, 23 @};}).
>>>>>>>
>>>>>>> + at item
>>>>>>> +for loops with variable definition (@samp{for (int i = 0; i < 8;
>>>> i++)});
>>>>>>> +
>>>>>>>  @item
>>>>>>>  Implementation defined behavior for signed integers is assumed to
>>>> match
>>>>>> the
>>>>>>>  expected behavior for two's complement. Non representable values in
>>>>>> integer
>>>>>>>
>>>>>>
>>>>>> IMO if you want this it would be better to just allow mixed statements
>>>> and
>>>>>> declarations, with this as a consequence.
>>>>>>
>>>>>> Can you comment on what the consequences would be for platform
>> support?
>>>>>> It would remove support for at least one platform I know of (the TI
>> ARM
>>>>>> compiler).  I've no idea whether it or any other platform which would
>> be
>>>>>> broken has any users, though.
>>>>>>
>>>>>
>>>>> No, I'm kind of against mixed statements and declarations, as are many
>>>>> people here. It mostly does make the code look worse and encourages
>>>> overuse
>>>>> of variables.
>>>>
>>>> I think the opposite.  I find declaration inside the loop header ugly
>> and
>>>> weird, while mixed declarations and code do make sense in some cases:
>> e.g.
>>>> pointer chasing through structures with different types - declaring all
>> the
>>>> variables in advance is just annoying.  (Maybe that's not strong enough
>> to
>>>> allow it everywhere if you believe that people will use it
>> inappropriately
>>>> though.)
>>>>
>>>>
>>> I'm pretty sure its because you're not used to them yet. I'm not taking
>>> this as a nak.
>>
>> I have been known to work on other codebases occasionally.  But yes,
>> that's just a style opinion and is not a nak (the below issue definitely is
>> without further work, though).
>>
>>> If you want mixed declaration submit a patch later on and let people
>>> comment on it.
>>
>> I think I dislike declarations inside loop headers sufficiently that I
>> would prefer the current state to having both.
>>
>>>>> I'm absoultely sure no compiler worth supporting would be broken. If
>> any
>>>>> +15 year old ones do get broken it would be well worth because it would
>>>>> still ease maintaining a lot. I'm also quite sure no compiler that
>> would
>>>> be
>>>>> broken by this would support compiling ffmpeg at all.
>>>>> This is still an essential feature of C99 after all and we don't use
>> C89.
>>>> TI at least disagrees with you, releases are still made without full C99
>>>> support.  I know it certainly has use on embedded platforms (though
>> likely
>>>> C6000 more so than ARM), but I don't know if anyone builds libavcodec or
>>>> similar with it.  (I don't use it - I only know this because Diego
>> recently
>>>> asked if anyone could test whether it could build libav, and I verified
>>>> that it could after fixing some minor issues.  He couldn't find any
>> users,
>>>> though, and the support was removed anyway.)
>>>>
>>>>
>>> If you're not aware of anyone compiling ffmpeg with it then don't even
>>> bother mentioning it.
>>
>> I stongly object to your dismissive response to this concern.  Your patch
>> is proposing immediately removing (not deprecating with intent to remove
>> later) support for an some unclear set of platforms, and it wasn't even
>> mentioned in the commit message or the mail.  I have noted one platform
>> which would be so removed, Carl noted one as well (I think he was referring
>> to <https://trac.ffmpeg.org/ticket/6728>), and there might be others.
>>
>> Before continuing with this patch I think you should at least:
>> * Have some idea what platforms are affected.
>> * Investigate whether these platforms have any significant user base
>> (maybe ask the user mailing lists, at least).
>> * Propose a patch to configure which either removes support for them or
>> somehow disables them (e.g. it could test-compile a loop including a
>> declaration).
>>
>>
> You aren't clear on how many platforms are affected either so I object to
> your objection to my dismissiveness and making it seem like its a big deal.
> I have some idea about which platofrms are affected and I'd be implicitly
> dropping support for them, so that's 1 and 3 off.

3 requires the removal of a configure line, as i said in my previous
email. There's nothing implicit about it with this documentation patch
alone.


More information about the ffmpeg-devel mailing list