[FFmpeg-devel] [PATCH] doc/developer: add examples to clarify code style

[Andrew Sayers]

I would have found this very helpful!

On Sat, May 18, 2024 at 07:00:50PM +0200, Marvin Scholz wrote:
> Given the frequency that new developers, myself included, get the
> code style wrong, it is useful to add some examples to clarify how
> things should be done.
> ---
>  doc/developer.texi | 57 +++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 56 insertions(+), 1 deletion(-)
> 
> diff --git a/doc/developer.texi b/doc/developer.texi
> index 63835dfa06..d7bf3f9cb8 100644
> --- a/doc/developer.texi
> +++ b/doc/developer.texi
> @@ -115,7 +115,7 @@ Objective-C where required for interacting with macOS-specific interfaces.
>  
>  @section Code formatting conventions
>  
> -There are the following guidelines regarding the indentation in files:
> +There are the following guidelines regarding the code style in files:
>  
>  @itemize @bullet
>  @item
> @@ -135,6 +135,61 @@ K&R coding style is used.
>  @end itemize
>  The presentation is one inspired by 'indent -i4 -kr -nut'.
>  
> + at subsection Examples
> +Some notable examples to illustrate common code style in FFmpeg:
> +
> + at itemize @bullet
> +
> + at item
> +Spaces around @code{if}/@code{do}/@code{while}/@code{for} conditions and assigments:

s/assigments/assignments/

Also, this might be more readable as "Space around assignments and after
@code{if}... keywords"?  On first pass, I assumed this was telling me
`( condition )` is correct, then had to re-read when the example showed
it wasn't.

> +
> + at example c
> +if (condition)

`condition` here differs from `cond` below, despite conveying the same meaning.
Either word is fine so long as it's the same word in both places.

> +    av_foo();
> + at end example
> +
> + at example c
> +for (size_t i = 0; i < len; i++)

This lightly implies we prefer `i < len` over `i != len` and `i++` over `++i`.
Is that something people round here have strong opinions about?  Maybe iterate
over a linked list if this is a controversial question?

> +    av_bar(i);
> + at end example
> +
> + at example c
> +size_t size = 0;
> + at end example
> +
> +However no spaces between the parentheses and condition, unless it helps
> +readability of complex conditions, so the following should not be done:
> +
> + at example c
> +// Wrong:

Nitpick: if you're going to say "// Wrong" here, it might be better to introduce
the mechanism with some "// Good"s or something above.  The consistency reduces
cognitive load on the learner, and it's a good excuse to add a little positivity
to a nerve-wracking experience.

> +if ( cond )
> +    av_foo();
> + at end example
> +
> + at item
> +No unnecessary parentheses, unless it helps readability:
> +
> + at example c
> +flags = s->mb_x ? RIGHT_EDGE : LEFT_EDGE | RIGHT_EDGE;
> + at end example

Can the example use "+" or "*" instead of "|"?  I've had so many bugs where
I got the precedence wrong, I'm not sure whether this is supposed to be a good
or bad example of readability.

> +
> + at item
> +No braces around single-line blocks:
> +
> + at example c
> +if (bits_pixel == 24)
> +    avctx->pix_fmt = AV_PIX_FMT_BGR24;
> +else if (bits_pixel == 8)
> +    avctx->pix_fmt = AV_PIX_FMT_GRAY8;
> +else @{
> +    av_log(avctx, AV_LOG_ERROR, "Invalid pixel format.\n");
> +    return AVERROR_INVALIDDATA;
> +@}
> + at end example
> +
> + at end itemize
> +
> +
>  @subsection Vim configuration
>  In order to configure Vim to follow FFmpeg formatting conventions, paste
>  the following snippet into your @file{.vimrc}:

Some other things that could help (in decreasing order of importance)...

* if you find a piece of code that looks wrong, should you...
  a) ignore the guide and match your style to the surroundings?
  b) follow the guide and accept the file will look inconsistent?
  c) add an extra patch to fix the formatting?

(I suspect the answer is (b), but could well be wrong)

* example of brace style for both functions and structs
  (as a newbie you don't know if you're about to meet one of those people
  who get all bent out of shape when they see a bracket on a line on its own
  )

* prefer `foo=bar; if (foo)` over `if ((foo=bar))`
  (the latter is sadly used in the code, but is a speedbump for reviewers)

* `foo *bar`, not `foo* bar`
  (I always forget this, not important if it's just me)

Also, way outside the scope of this patch, but a linter that checks these things
would be very much appreciated.  There's a lot to get wrong with your first patch,
and a program that just said "yep that's formatted correctly" might save a newbie
enough time to learn git-send-email instead.