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

[Stefano Sabatini]

On date Wednesday 2025-05-07 23:59:39 +0000, softworkz . wrote:
> 
> 
> > -----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.
> 

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.