[FFmpeg-devel] Curly braces around single statements (was avformat/avidec: Fix memleak when error) happens after creating DV stream

[Alexander Strasser]

Hi Mark,

thanks for your opinion on this!

On 2020-08-20 23:35 +0100, Mark Thompson wrote:
> On 20/08/2020 22:20, Alexander Strasser wrote:
> > Please pardon me for bringing this up in the context of this patch.
> > No objections or particular opinion regarding this instance of the
> > problem.
> >
> > Though thinking more globally, I believe it would have a beneficial
> > impact to add the curly braces everywhere; even where they would not
> > be required because of the one-statement exception.
> >
> > It might be a bit longer regarding vertical space consumption, but
> > I'm sure the advantages would outweigh this disadvantage over time.
> > Also because we don't put the opening brace on a line of its own,
> > it would not consume so much more vertical space.
> >
> > Advantages I see are:
> >
> > 1. enables easier experimentation and debugging
>
> In my opinion this is not a significant benefit, the overhead of any edits while doing this is trivial.  (Others may disagree, but it seems worth noting which parts I agree with.)

I obviously disagree here, but it's sure a subjective topic.


> > 2. future changes are easier to write and create smaller diffs
>
> I agree that this is a point in favour.
>
> > 3. completely eliminates dangling-else problems
>
> I think this is irrelevant, because compilers have already solved it:
>
> $ cat test-dangling-else.c
>
> int f(int a, int b)
> {
>     if (a)
>         if (b)
>             return 1;
>       else
>           return 2;
>
>     return 0;
> }
> $ gcc -c -Wall test-dangling-else.c
> test-dangling-else.c: In function ‘f’:
> test-dangling-else.c:4:8: warning: suggest explicit braces to avoid ambiguous ‘else’ [-Wdangling-else]
>     4 |     if (a)
>       |        ^

Agreed, compilers warn about this. I  still think there is merit in
being able sort it out, before compiling, especially when doing code
reviews on the mailing list.


> > Just wanted to hear how other developers currently feel about this.
>
> The freeform nature of C is often helpful to make code look nicer and such a requirement would make some useful patterns worse, so I oppose such a change.
>
> To offer an example, consider the commonish code pattern:
>
> if      (a) p = x;
> else if (b) q = y;
> else if (c) r = z;

I think this sort of alignment is another practice that didn't stand
the test of time.

I know it is very tempting to use when initially adding some code,
but it is often subjective and hard to keep in shape after (multiple)
change over times. You need to follow up with cosmetics and I assume
more often than not it will be overseen or forgotten, which makes
that code weird to read.

I know this sort of alignment is used often throughout FFmpeg's
code base. If you want I can try to dig out examples where it has
gone wrong, stumbled over this every now and then.


  Alexander

>
> with some alignment of related subexpressions.  Making uniform use of braces and newlines mandatory makes this actively worse however you do it.
>
> (Some randomly-chosen similar examples: <http://git.videolan.org/?p=ffmpeg.git;a=blob;f=libavcodec/mpegaudiodsp_template.c;h=e531f8a904b14ad2f5cc6e59ad608bfe64b50065;hb=HEAD#l236>, <http://git.videolan.org/?p=ffmpeg.git;a=blob;f=libavcodec/error_resilience.c;h=ca2287198be69505a03f12dcbe9cff2b485de769;hb=HEAD#l465>.)