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

[epirat07 at gmail.com]

On 18 May 2024, at 20:22, Andrew Sayers wrote:

> I would have found this very helpful!

Thanks for the review, I just submitted a new version that hopefully
addresses most of your points.

>
> 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.
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".