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

[Carl Eugen Hoyos]

2018-05-18 3:41 GMT+02:00, Rostislav Pehlivanov :

> Pushed, thanks
> Its a bittersweet victory, took 3 years but its finally done.

Is it possible that you don't see that a "victory" always has
a disadvantage?

Carl Eugen
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Rostislav Pehlivanov]

On 8 November 2017 at 21:26, Rostislav Pehlivanov 
wrote:

> Signed-off-by: Rostislav Pehlivanov 
> ---
>  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 @};}).
>
> +@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
> --
> 2.15.0.403.gc27cc4dac6
>
>
Pushed, thanks
Its a bittersweet victory, took 3 years but its finally done.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Rostislav Pehlivanov]

On 14 May 2018 at 23:59, Hendrik Leppkes  wrote:

> On Tue, May 15, 2018 at 12:30 AM, Rostislav Pehlivanov
>  wrote:
> > On 14 May 2018 at 22:57, Mark Thompson  wrote:
> >
> >> On 14/05/18 22:38, Rostislav Pehlivanov wrote:
> >> > On 12 May 2018 at 20:49, Rostislav Pehlivanov 
> >> wrote:
> >> >> On 8 November 2017 at 21:26, Rostislav Pehlivanov <
> atomnu...@gmail.com>
> >> >> wrote:
> >> >>
> >> >>> Signed-off-by: Rostislav Pehlivanov 
> >> >>> ---
> >> >>>  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 @};}).
> >> >>>
> >> >>> +@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
> >> >>> --
> >> >>> 2.15.0.403.gc27cc4dac6
> >> >>>
> >> >>>
> >> >> Ping.
> >> >> Apparently we don't support old msvc versions, so there's nothing
> >> stopping
> >> >> us from using them.
> >> >>
> >> >
> >> > I'll apply this tomorrow unless there are any objections and will
> apply
> >> > some patches to convert some of my code to this.
> >>
> >> On 08/11/17 23:05, Mark Thompson wrote:
> >> > 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).
> >>
> >> Are these done?
> >>
> >
> > Yes, the only platforms mentioned were MSVC versions older than 2013,
> which
> > are unsupported as of recent.
>
> How is that? MSVC 2013 still works mostly fine (short of one test
> where the Unicode handling is a bit weird, and noone bothered to work
> around it).
>
> - Hendrik
> ___
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>

MSVC 2013 is still perfectly supported and tested, this is for versions
older than that (when they didn't have proper C99 support).
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Hendrik Leppkes]

On Tue, May 15, 2018 at 12:30 AM, Rostislav Pehlivanov
 wrote:
> On 14 May 2018 at 22:57, Mark Thompson  wrote:
>
>> On 14/05/18 22:38, Rostislav Pehlivanov wrote:
>> > On 12 May 2018 at 20:49, Rostislav Pehlivanov 
>> wrote:
>> >> On 8 November 2017 at 21:26, Rostislav Pehlivanov 
>> >> wrote:
>> >>
>> >>> Signed-off-by: Rostislav Pehlivanov 
>> >>> ---
>> >>>  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 @};}).
>> >>>
>> >>> +@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
>> >>> --
>> >>> 2.15.0.403.gc27cc4dac6
>> >>>
>> >>>
>> >> Ping.
>> >> Apparently we don't support old msvc versions, so there's nothing
>> stopping
>> >> us from using them.
>> >>
>> >
>> > I'll apply this tomorrow unless there are any objections and will apply
>> > some patches to convert some of my code to this.
>>
>> On 08/11/17 23:05, Mark Thompson wrote:
>> > 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).
>>
>> Are these done?
>>
>
> Yes, the only platforms mentioned were MSVC versions older than 2013, which
> are unsupported as of recent.

How is that? MSVC 2013 still works mostly fine (short of one test
where the Unicode handling is a bit weird, and noone bothered to work
around it).

- Hendrik
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Mark Thompson]

On 14/05/18 23:30, Rostislav Pehlivanov wrote:
> On 14 May 2018 at 22:57, Mark Thompson  wrote:
>> On 14/05/18 22:38, Rostislav Pehlivanov wrote:
>>> On 12 May 2018 at 20:49, Rostislav Pehlivanov 
>> wrote:
 On 8 November 2017 at 21:26, Rostislav Pehlivanov 
 wrote:

> Signed-off-by: Rostislav Pehlivanov 
> ---
>  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 @};}).
>
> +@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
> --
> 2.15.0.403.gc27cc4dac6
>
>
 Ping.
 Apparently we don't support old msvc versions, so there's nothing
>> stopping
 us from using them.

>>>
>>> I'll apply this tomorrow unless there are any objections and will apply
>>> some patches to convert some of my code to this.
>>
>> On 08/11/17 23:05, Mark Thompson wrote:
>>> 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).
>>
>> Are these done?
>>
> 
> Yes, the only platforms mentioned were MSVC versions older than 2013, which
> are unsupported as of recent.

Do you have a link to this discussion?  I'm not sure I've seen it.

Where can I find the configure patch?

- Mark
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Rostislav Pehlivanov]

On 14 May 2018 at 22:57, Mark Thompson  wrote:

> On 14/05/18 22:38, Rostislav Pehlivanov wrote:
> > On 12 May 2018 at 20:49, Rostislav Pehlivanov 
> wrote:
> >> On 8 November 2017 at 21:26, Rostislav Pehlivanov 
> >> wrote:
> >>
> >>> Signed-off-by: Rostislav Pehlivanov 
> >>> ---
> >>>  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 @};}).
> >>>
> >>> +@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
> >>> --
> >>> 2.15.0.403.gc27cc4dac6
> >>>
> >>>
> >> Ping.
> >> Apparently we don't support old msvc versions, so there's nothing
> stopping
> >> us from using them.
> >>
> >
> > I'll apply this tomorrow unless there are any objections and will apply
> > some patches to convert some of my code to this.
>
> On 08/11/17 23:05, Mark Thompson wrote:
> > 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).
>
> Are these done?
>

Yes, the only platforms mentioned were MSVC versions older than 2013, which
are unsupported as of recent.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Mark Thompson]

On 14/05/18 22:38, Rostislav Pehlivanov wrote:
> On 12 May 2018 at 20:49, Rostislav Pehlivanov  wrote:
>> On 8 November 2017 at 21:26, Rostislav Pehlivanov 
>> wrote:
>>
>>> Signed-off-by: Rostislav Pehlivanov 
>>> ---
>>>  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 @};}).
>>>
>>> +@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
>>> --
>>> 2.15.0.403.gc27cc4dac6
>>>
>>>
>> Ping.
>> Apparently we don't support old msvc versions, so there's nothing stopping
>> us from using them.
>>
> 
> I'll apply this tomorrow unless there are any objections and will apply
> some patches to convert some of my code to this.

On 08/11/17 23:05, Mark Thompson wrote:
> 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).

Are these done?

- Mark
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Rostislav Pehlivanov]

On 12 May 2018 at 20:49, Rostislav Pehlivanov  wrote:

>
>
> On 8 November 2017 at 21:26, Rostislav Pehlivanov 
> wrote:
>
>> Signed-off-by: Rostislav Pehlivanov 
>> ---
>>  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 @};}).
>>
>> +@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
>> --
>> 2.15.0.403.gc27cc4dac6
>>
>>
> Ping.
> Apparently we don't support old msvc versions, so there's nothing stopping
> us from using them.
>

I'll apply this tomorrow unless there are any objections and will apply
some patches to convert some of my code to this.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Rostislav Pehlivanov]

On 8 November 2017 at 21:26, Rostislav Pehlivanov 
wrote:

> Signed-off-by: Rostislav Pehlivanov 
> ---
>  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 @};}).
>
> +@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
> --
> 2.15.0.403.gc27cc4dac6
>
>
Ping.
Apparently we don't support old msvc versions, so there's nothing stopping
us from using them.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Clément Bœsch]

On Fri, Nov 10, 2017 at 12:18:07AM +0100, Aurelien Jacobs wrote:
[...]
> > >   Also, allowing this but not the mixed statements and declarations means
> > >   this is a style decision and not a technical one anymore.
> 
> Allowing limiting the scope of a variable to a loop seems like a
> technical decision to me, but I understand that some consider it more
> of a style decision.
> 

What I meant by that is that currently the for (int ..) form is prevented
by style but also technical argument (compiler compatibility). If you
remove the technical argument by breaking the compiler compatibility of
such form, what remains is only a style choice to make between mixed
stat/decl and for (int ...); AFAIK there is no difference for a compiler
between the two.

-- 
Clément B.


signature.asc
Description: PGP signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Aurelien Jacobs]

On Thu, Nov 09, 2017 at 04:56:07PM -0300, James Almer wrote:
> On 11/9/2017 2:46 PM, Clément Bœsch wrote:
> > On Wed, Nov 08, 2017 at 09:26:13PM +, Rostislav Pehlivanov wrote:
> >> Signed-off-by: Rostislav Pehlivanov 
> >> ---
> >>  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 @};}).
> >>  
> >> +@item
> >> +for loops with variable definition (@samp{for (int i = 0; i < 8; i++)});
> >> +
> > 
> > I'm fine with this and would be happy to update all the code I maintain in
> > FFmpeg to follow this pattern. But I have a few questions/reservations:
> > 
> > - what does it imply for mixed statements and declarations?

Nothing at all. It is unrelated to mixed statements and declarations.

> >   If we still do not allow them, how are you going to make the compiler
> >   reject them but not the for (int ... ) form?

By not changing anything at all. That is already what happens right now.

> No way to do that i guess. If we remove the warning, it will never catch
> any kind of mixed declarations and statements anymore.

No need to remove any warning. The -Wdeclaration-after-statement does
not emit warnings regarding for loops with declarations.

> >   Also, allowing this but not the mixed statements and declarations means
> >   this is a style decision and not a technical one anymore.

Allowing limiting the scope of a variable to a loop seems like a
technical decision to me, but I understand that some consider it more
of a style decision.

> > [...]
> > 
> > Overall, I'd enjoy this change being accepted (even along mixed statements
> > and declarations).

Just for the record, I would enjoy it as well.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[James Almer]

On 11/9/2017 5:08 PM, Clément Bœsch wrote:
> On Thu, Nov 09, 2017 at 04:56:07PM -0300, James Almer wrote:
> [...]
>>> - this require a Changelog entry as it has a technical impact (which could
>>>   be considered negligible).
>>
>> No, Changelog is not for this kind of change.
>>
> 
> Sorry, I should have elaborated: I meant to document in the Changelog the
> drop of support for the specific affected compilers.

Oh right. In that case i guess yes, we did add lines about support for
new compilers there after all.

But still, Mark's concerns should be addressed before anything is decided.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Clément Bœsch]

On Thu, Nov 09, 2017 at 04:56:07PM -0300, James Almer wrote:
[...]
> > - this require a Changelog entry as it has a technical impact (which could
> >   be considered negligible).
> 
> No, Changelog is not for this kind of change.
> 

Sorry, I should have elaborated: I meant to document in the Changelog the
drop of support for the specific affected compilers.

-- 
Clément B.


signature.asc
Description: PGP signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[James Almer]

On 11/9/2017 2:46 PM, Clément Bœsch wrote:
> On Wed, Nov 08, 2017 at 09:26:13PM +, Rostislav Pehlivanov wrote:
>> Signed-off-by: Rostislav Pehlivanov 
>> ---
>>  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 @};}).
>>  
>> +@item
>> +for loops with variable definition (@samp{for (int i = 0; i < 8; i++)});
>> +
> 
> I'm fine with this and would be happy to update all the code I maintain in
> FFmpeg to follow this pattern. But I have a few questions/reservations:
> 
> - what does it imply for mixed statements and declarations?
> 
>   If we still do not allow them, how are you going to make the compiler
>   reject them but not the for (int ... ) form?

No way to do that i guess. If we remove the warning, it will never catch
any kind of mixed declarations and statements anymore.

> 
>   Also, allowing this but not the mixed statements and declarations means
>   this is a style decision and not a technical one anymore.

Expanding on the above, doing this will become hard to enforce without
the warning, unless people reviewing patches start looking very closely
for them.

> 
> - are we going to accept all kind of patches to change the coding style
>   all over the codebase?

I'd rather not allow that. Big cosmetics changes tend to make using git
blame a PITA.

> 
> - this require a Changelog entry as it has a technical impact (which could
>   be considered negligible).

No, Changelog is not for this kind of change.

Maybe we should add some sort of changelog specific for doc/developer or
similar documentation.

> 
> Overall, I'd enjoy this change being accepted (even along mixed statements
> and declarations).

I personally find the current style cleaner looking, but i wouldn't
oppose this if it can be proven we're not breaking compilers currently
supported (and still used) just for it, as Mark requested in another email.
Afaik, MSVC 2012 and older stopped working with ffmpeg some time ago, or
at least are not tested by FATE any more.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Clément Bœsch]

On Wed, Nov 08, 2017 at 09:26:13PM +, Rostislav Pehlivanov wrote:
> Signed-off-by: Rostislav Pehlivanov 
> ---
>  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 @};}).
>  
> +@item
> +for loops with variable definition (@samp{for (int i = 0; i < 8; i++)});
> +

I'm fine with this and would be happy to update all the code I maintain in
FFmpeg to follow this pattern. But I have a few questions/reservations:

- what does it imply for mixed statements and declarations?

  If we still do not allow them, how are you going to make the compiler
  reject them but not the for (int ... ) form?

  Also, allowing this but not the mixed statements and declarations means
  this is a style decision and not a technical one anymore.

- are we going to accept all kind of patches to change the coding style
  all over the codebase?

- this require a Changelog entry as it has a technical impact (which could
  be considered negligible).

Overall, I'd enjoy this change being accepted (even along mixed statements
and declarations).

[...]

-- 
Clément B.


signature.asc
Description: PGP signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[James Almer]

On 11/8/2017 9:39 PM, Rostislav Pehlivanov wrote:
> On 8 November 2017 at 23:05, Mark Thompson  wrote:
> 
>> On 08/11/17 22:41, Rostislav Pehlivanov wrote:
>>> On 8 November 2017 at 22:20, Mark Thompson  wrote:
>>>
 On 08/11/17 22:03, Rostislav Pehlivanov wrote:
> On 8 November 2017 at 21:49, Mark Thompson  wrote:
>
>> On 08/11/17 21:26, Rostislav Pehlivanov wrote:
>>> Signed-off-by: Rostislav Pehlivanov 
>>> ---
>>>  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 @};}).
>>>
>>> +@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 ), 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 

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

[Rostislav Pehlivanov]

On 8 November 2017 at 23:05, Mark Thompson  wrote:

> On 08/11/17 22:41, Rostislav Pehlivanov wrote:
> > On 8 November 2017 at 22:20, Mark Thompson  wrote:
> >
> >> On 08/11/17 22:03, Rostislav Pehlivanov wrote:
> >>> On 8 November 2017 at 21:49, Mark Thompson  wrote:
> >>>
>  On 08/11/17 21:26, Rostislav Pehlivanov wrote:
> > Signed-off-by: Rostislav Pehlivanov 
> > ---
> >  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 @};}).
> >
> > +@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 ), 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.
___

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

[Rostislav Pehlivanov]

On 8 November 2017 at 23:09, James Almer  wrote:

> On 11/8/2017 7:41 PM, Rostislav Pehlivanov wrote:
> > On 8 November 2017 at 22:20, Mark Thompson  wrote:
> >
> >> On 08/11/17 22:03, Rostislav Pehlivanov wrote:
> >>> On 8 November 2017 at 21:49, Mark Thompson  wrote:
> >>>
>  On 08/11/17 21:26, Rostislav Pehlivanov wrote:
> > Signed-off-by: Rostislav Pehlivanov 
> > ---
> >  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 @};}).
> >
> > +@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.
> > If you want mixed declaration submit a patch later on and let people
> > comment on it.
>
> It's the other way around. If you want to introduce some change, you're
> the one that needs to convince other devs it's a good change, and so
> far, two dislike it.
> You can't commit this when people are against it saying "send a patch to
> undo it later".
>


What two people, Carl hasn't said he's against it.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[James Almer]

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  wrote:
>>
>>> On 08/11/17 22:03, Rostislav Pehlivanov wrote:
 On 8 November 2017 at 21:49, Mark Thompson  wrote:

> On 08/11/17 21:26, Rostislav Pehlivanov wrote:
>> Signed-off-by: Rostislav Pehlivanov 
>> ---
>>  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 @};}).
>>
>> +@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 
> ), 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@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 

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

[Mark Thompson]

On 08/11/17 22:41, Rostislav Pehlivanov wrote:
> On 8 November 2017 at 22:20, Mark Thompson  wrote:
> 
>> On 08/11/17 22:03, Rostislav Pehlivanov wrote:
>>> On 8 November 2017 at 21:49, Mark Thompson  wrote:
>>>
 On 08/11/17 21:26, Rostislav Pehlivanov wrote:
> Signed-off-by: Rostislav Pehlivanov 
> ---
>  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 @};}).
>
> +@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 
), 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,

- Mark
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[James Almer]

On 11/8/2017 7:41 PM, Rostislav Pehlivanov wrote:
> On 8 November 2017 at 22:20, Mark Thompson  wrote:
> 
>> On 08/11/17 22:03, Rostislav Pehlivanov wrote:
>>> On 8 November 2017 at 21:49, Mark Thompson  wrote:
>>>
 On 08/11/17 21:26, Rostislav Pehlivanov wrote:
> Signed-off-by: Rostislav Pehlivanov 
> ---
>  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 @};}).
>
> +@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.
> If you want mixed declaration submit a patch later on and let people
> comment on it.

It's the other way around. If you want to introduce some change, you're
the one that needs to convince other devs it's a good change, and so
far, two dislike it.
You can't commit this when people are against it saying "send a patch to
undo it later".

Besides, this patch alone is incomplete. Warnings about mixed code and
declaration are currently force enabled in configure.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Rostislav Pehlivanov]

On 8 November 2017 at 22:20, Mark Thompson  wrote:

> On 08/11/17 22:03, Rostislav Pehlivanov wrote:
> > On 8 November 2017 at 21:49, Mark Thompson  wrote:
> >
> >> On 08/11/17 21:26, Rostislav Pehlivanov wrote:
> >>> Signed-off-by: Rostislav Pehlivanov 
> >>> ---
> >>>  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 @};}).
> >>>
> >>> +@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.
If you want mixed declaration submit a patch later on and let people
comment on it.




> > 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.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Mark Thompson]

On 08/11/17 22:03, Rostislav Pehlivanov wrote:
> On 8 November 2017 at 21:49, Mark Thompson  wrote:
> 
>> On 08/11/17 21:26, Rostislav Pehlivanov wrote:
>>> Signed-off-by: Rostislav Pehlivanov 
>>> ---
>>>  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 @};}).
>>>
>>> +@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 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.)

- Mark
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Rostislav Pehlivanov]

On 8 November 2017 at 21:49, Mark Thompson  wrote:

> On 08/11/17 21:26, Rostislav Pehlivanov wrote:
> > Signed-off-by: Rostislav Pehlivanov 
> > ---
> >  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 @};}).
> >
> > +@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.
>
> - Mark
> ___
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>


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'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.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Carl Eugen Hoyos]

2017-11-08 22:49 GMT+01:00 Mark Thompson :
> On 08/11/17 21:26, Rostislav Pehlivanov wrote:

>> +@item
>> +for loops with variable definition (@samp{for (int i = 0; i < 8; i++)});

> Can you comment on what the consequences would be for platform support?

A similar-looking issue was reported for some version of msvc a short time ago.

Carl Eugen
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Rostislav Pehlivanov]

On 8 November 2017 at 21:45, Carl Eugen Hoyos  wrote:

> 2017-11-08 22:26 GMT+01:00 Rostislav Pehlivanov :
>
> > +@item
> > +for loops with variable definition (@samp{for (int i = 0; i < 8; i++)});
>
> Don't you think that this makes the code slightly uglier?
>
> Carl Eugen
> ___
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>



Definitely not, and here are some reasons why, in order of importance:

1.) Gives a very very clear indication of whether a variable is used after
a for loop. Any loop without its variable defined means that variable is
used for something else so everyone would pay more attention.

2.) It saves a line in case the loop variable isn't used anywhere
2a.) This adds up and if all loops in our codebase were converted many 10s
of thousands of lines would be saved (I'm serious, at least this many
lines).

3.) Simplifies experimenting since you don't have to scroll up to add a
variable and then scroll down to use it in a loop.

4.) Most code editing (and general purpose text editing) software
highlights types, so on a glance there would be no doubt about which
bracketed segment is a loop and which is a condition.

5.) Compilers can better know whether to unroll loops in case the variable
isn't actually used in a small number of cases.

6.) Simple compilers would make less allocation/stack mistakes if a
variable is declared in the first loop in a two loop segment.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Mark Thompson]

On 08/11/17 21:26, Rostislav Pehlivanov wrote:
> Signed-off-by: Rostislav Pehlivanov 
> ---
>  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 @};}).
>  
> +@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.

- Mark
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Carl Eugen Hoyos]

2017-11-08 22:26 GMT+01:00 Rostislav Pehlivanov :

> +@item
> +for loops with variable definition (@samp{for (int i = 0; i < 8; i++)});

Don't you think that this makes the code slightly uglier?

Carl Eugen
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Rostislav Pehlivanov]

Signed-off-by: Rostislav Pehlivanov 
---
 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 @};}).
 
+@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
-- 
2.15.0.403.gc27cc4dac6

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel