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

James Almer jamrial at gmail.com
Thu Nov 9 01:24:22 EET 2017


On 11/8/2017 8:05 PM, Mark Thompson 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).
> 
> Thanks,

libav directly makes gcc error out with mixed declaration and code,
whereas we only warn about it, so I'd say there are definitely some
systems or compilers out there where that's been an issue. MSVC <= 2012
is most assuredly one.

> 
> - Mark
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 



More information about the ffmpeg-devel mailing list