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

["softworkz ." <softworkz-at-hotmail.com@ffmpeg.org>]

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