[FFmpeg-devel] [PATCH v10 01/15] fftools/textformat: Formatting and whitespace changes

[softworkz .]

> -----Original Message-----
> From: Stefano Sabatini <stefasab at gmail.com>
> Sent: Donnerstag, 8. Mai 2025 01:45
> To: FFmpeg development discussions and patches <ffmpeg-devel at ffmpeg.org>
> Cc: softworkz <softworkz at 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 <softworkz at hotmail.com>
> >
> > Reviewed-by: Stefano Sabatini <stefasab at gmail.com>
> > Signed-off-by: softworkz <softworkz at 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.

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.

Thanks,
sw