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'.
>>
>> +@subsection Examples
>> +Some notable examples to illustrate common code style in FFmpeg:
>> +
>> +@itemize @bullet
>> +
>> +@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.
>
>> +
>> +@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();
>> +@end example
>> +
>> +@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);
>> +@end example
>> +
>> +@example c
>> +size_t size = 0;
>> +@end example
>> +
>> +However no spaces between the parentheses and condition, unless it helps
>> +readability of complex conditions, so the following should not be done:
>> +
>> +@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();
>> +@end example
>> +
>> +@item
>> +No unnecessary parentheses, unless it helps readability:
>> +
>> +@example c
>> +flags = s->mb_x ? RIGHT_EDGE : LEFT_EDGE | RIGHT_EDGE;
>> +@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.
>
>> +
>> +@item
>> +No braces around single-line blocks:
>> +
>> +@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;
>> +@}
>> +@end example
>> +
>> +@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@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
