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

Rostislav Pehlivanov atomnuker at gmail.com
Thu Nov 9 02:39:07 EET 2017


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.


More information about the ffmpeg-devel mailing list