From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from ffbox0-bg.mplayerhq.hu (ffbox0-bg.ffmpeg.org [79.124.17.100]) by master.gitmailbox.com (Postfix) with ESMTP id C0FD34AC7D for ; Sat, 18 May 2024 18:22:25 +0000 (UTC) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id 8369768CDB0; Sat, 18 May 2024 21:22:22 +0300 (EEST) Received: from alt2.a-painless.mh.aa.net.uk (alt2.a-painless.mh.aa.net.uk [81.187.30.51]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id 17F9968C825 for ; Sat, 18 May 2024 21:22:16 +0300 (EEST) Received: from 5.8.8.f.8.c.a.9.0.0.1.0.f.6.d.c.0.5.8.0.9.1.8.0.0.b.8.0.1.0.0.2.ip6.arpa ([2001:8b0:819:850:cd6f:100:9ac8:f885] helo=andrews-2024-laptop.sayers) by painless-a.thn.aa.net.uk with smtp (Exim 4.96) (envelope-from ) id 1s8OhD-009N6j-0q for ffmpeg-devel@ffmpeg.org; Sat, 18 May 2024 19:22:15 +0100 Date: Sat, 18 May 2024 19:22:08 +0100 From: Andrew Sayers To: FFmpeg development discussions and patches Message-ID: References: MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: Subject: Re: [FFmpeg-devel] [PATCH] doc/developer: add examples to clarify code style X-BeenThere: ffmpeg-devel@ffmpeg.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: FFmpeg development discussions and patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: FFmpeg development discussions and patches Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: ffmpeg-devel-bounces@ffmpeg.org Sender: "ffmpeg-devel" Archived-At: List-Archive: List-Post: 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'. > > +@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-request@ffmpeg.org with subject "unsubscribe".