> -----Original Message-----
> From: Stefano Sabatini <stefa...@gmail.com>
> Sent: Donnerstag, 8. Mai 2025 02:14
> To: softworkz . <softwo...@hotmail.com>
> Cc: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH v10 01/15] fftools/textformat: Formatting
> and whitespace changes
>
> On date Wednesday 2025-05-07 23:59:39 +0000, softworkz . wrote:
> >
> >
> > > -----Original Message-----
> > > From: Stefano Sabatini <stefa...@gmail.com>
> > > Sent: Donnerstag, 8. Mai 2025 01:45
> > > To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org>
> > > Cc: softworkz <softwo...@hotmail.com>
> > > Subject: Re: [FFmpeg-devel] [PATCH v10 01/15] fftools/textformat:
> Formatting
> > > and whitespace changes
> > >
> > > On date Sunday 2025-05-04 02:57:12 +0000, softworkz wrote:
> > > > From: softworkz <softwo...@hotmail.com>
> > > >
> > > > Reviewed-by: Stefano Sabatini <stefa...@gmail.com>
> > > > Signed-off-by: softworkz <softwo...@hotmail.com>
> > > > ---
> > > > fftools/textformat/avtextformat.c | 92 +++++++++++++++---------------
> > > > fftools/textformat/avtextformat.h | 20 +++----
> > > > fftools/textformat/avtextwriters.h | 11 ++--
> > > > fftools/textformat/tf_compact.c | 91 ++++++++++++++++-------------
> > > > fftools/textformat/tf_default.c | 20 +++----
> > > > fftools/textformat/tf_flat.c | 26 +++++----
> > > > fftools/textformat/tf_ini.c | 36 ++++++------
> > > > fftools/textformat/tf_json.c | 10 ++--
> > > > fftools/textformat/tf_xml.c | 30 +++++-----
> > > > 9 files changed, 177 insertions(+), 159 deletions(-)
> > > >
> > > [...]
> > > > - if (show_data_hash) {
> > > > + if (show_data_hash)
> > > > if ((ret = av_hash_alloc(&tctx->hash, show_data_hash)) < 0) {
> > > > if (ret == AVERROR(EINVAL)) {
> > > > const char *n;
> > > > @@ -211,7 +211,6 @@ int avtext_context_open(AVTextFormatContext **ptctx,
> > > const AVTextFormatter *form
> > > > }
> > > > return ret;
> > > > }
> > > > - }
> > >
> > > For the record, I'm a bit against these kind of changes since they do
> > > not really add to readability and might led to logical issues in case
> > > an instruction is added to the else block and the parentheses are
> > > discarded - but not so strongly opposed to block the patch though.
> > >
> > > [...]
> > > > -void avtext_print_section_header(AVTextFormatContext *tctx,
> > > > - const void *data,
> > > > - int section_id)
> > > > +void avtext_print_section_header(AVTextFormatContext *tctx, const void
> > > *data, int section_id)
> > > > {
> > > > tctx->level++;
> > > > av_assert0(tctx->level < SECTION_MAX_NB_LEVELS);
> > > > @@ -272,8 +269,9 @@ void avtext_print_section_header(AVTextFormatContext
> > > *tctx,
> > >
> > > > void avtext_print_section_footer(AVTextFormatContext *tctx)
> > > > {
> > > > int section_id = tctx->section[tctx->level]->id;
> > > > - int parent_section_id = tctx->level ?
> > > > - tctx->section[tctx->level-1]->id : SECTION_ID_NONE;
> > > > + int parent_section_id = tctx->level
> > > > + ? tctx->section[tctx->level - 1]->id
> > > > + : SECTION_ID_NONE;
> > >
> > > nit: I prefer the original form (it now looks less readable).
> > >
> > > [...]
> > > > if (section->flags & AV_TEXTFORMAT_SECTION_FLAG_HAS_TYPE) {
> > > > // add /TYPE to prefix
> > > > @@ -185,30 +189,33 @@ static void
> > > compact_print_section_header(AVTextFormatContext *wctx, const void *
> > > > char c =
> > > > (*p >= '0' && *p <= '9') ||
> > > > (*p >= 'a' && *p <= 'z') ||
> > > > - (*p >= 'A' && *p <= 'Z') ? av_tolower(*p) : '_';
> > > > + (*p >= 'A' && *p <= 'Z')
> > > > + ? (char)(char)av_tolower(*p)
> > > > + : '_';
> > >
> > > Ditto.
> > >
> > > [...]
> > >
> > > Should be good otherwise.
> >
> >
> > Hi Stefano,
> >
> > In message "[PATCH] [RFC] global/clang-format: Add .clang-format
> configuration for consistent formatting" I'm trying to work out a common
> format that matches the existing code formatting rules.
> >
> > The patches above are formatted accordingly.
> >
> > Regarding the first above, FFmpeg rules say:
> >
> > Don't wrap single-line blocks in braces. Use braces only if there is an
> accompanying else statement.
> > (https://ffmpeg.org/developer.html#Code-formatting-conventions)
> >
> > Now, I'm not sure whether this exactly means "single line" - clang-format
> only supports "single statement/block", so if it means "single line", it might
> be difficult to replicate.
> >
>
> So, as I wrote, I don't have strong objections against the brace
> removals if you want to go with that.
>
> > Seems, we don't have any rule for ternary operator placement, afaik,
> > the most common rule is to place them at the beginning of lines, but
> > I surely can revert it.
>
> About ternary, I'd drop those changes as they conflict with the style
> adopted in the whole FFmpeg codebase.
No worries, all changes already made as requested. 😊
sw
_______________________________________________
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".
