Re: [FFmpeg-devel] [PATCH 1/9] fftools/textformat: Formatting and whitespace changes

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

> -----Original Message-----
> From: Stefano Sabatini <stefasab@gmail.com>
> Sent: Montag, 21. April 2025 18:53
> To: FFmpeg development discussions and patches <ffmpeg-
> devel@ffmpeg.org>
> Cc: softworkz <softworkz@hotmail.com>
> Subject: Re: [FFmpeg-devel] [PATCH 1/9] fftools/textformat: Formatting
> and whitespace changes
> 
> On date Monday 2025-04-14 12:46:58 +0000, softworkz wrote:
> > From: softworkz <softworkz@hotmail.com>
> >
> > Signed-off-by: softworkz <softworkz@hotmail.com>
> > ---
> >  fftools/textformat/avtextformat.c  | 104 +++++++++++++++-----------
> ---
> >  fftools/textformat/avtextformat.h  |  16 ++---
> >  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, 183 insertions(+), 161 deletions(-)
> >
> 
> [...]
> > -int avtext_context_open(AVTextFormatContext **ptctx, const
> AVTextFormatter *formatter, AVTextWriterContext *writer_context, const
> char *args,
> > -                        const struct AVTextFormatSection *sections,
> int nb_sections,
> > -                        int show_value_unit,
> > -                        int use_value_prefix,
> > -                        int use_byte_value_binary_prefix,
> > -                        int use_value_sexagesimal_format,
> > -                        int show_optional_fields,
> > -                        char *show_data_hash)
> > +int avtext_context_open(AVTextFormatContext      **ptctx,
> > +                        const AVTextFormatter     *formatter,
> > +                        AVTextWriterContext       *writer_context,
> > +                        const char                *args,
> > +                        const AVTextFormatSection *sections,
> > +                        int                        nb_sections,
> > +                        int                        show_value_unit,
> > +                        int
> use_value_prefix,
> > +                        int
> use_byte_value_binary_prefix,
> > +                        int
> use_value_sexagesimal_format,
> > +                        int
> show_optional_fields,
> > +                        char                      *show_data_hash)
> 
> This is possibly one of the few places where we align
> parameters. Unrelated note: the function also has too many parameters,
> probably they should be merged in a flags field.

Or an options struct maybe?


> 
> >  {
> >      AVTextFormatContext *tctx;
> >      int i, ret = 0;
> > @@ -197,7 +201,7 @@ int avtext_context_open(AVTextFormatContext
> **ptctx, const AVTextFormatter *form
> >          av_dict_free(&opts);
> >      }
> >
> > -    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;
> > @@ -208,7 +212,6 @@ int avtext_context_open(AVTextFormatContext
> **ptctx, const AVTextFormatter *form
> >              }
> >              return ret;
> >          }
> > -    }
> 
> I'm a bit against dropping {} since this doesn't add to redability in
> most cases and can lead to bugs in case more statements are added in
> the if block (since it's easy to miss the wrapping parentheses
> addition).

I thought the rules would say that "if" blocks should only be parenthesized
when it has multiple statements OR there's an "else" clause.

Personally, I would always use parenthesizes, but that's of little 
relevance when the codebase has different rules.


> 
> [...]
> > @@ -81,6 +81,7 @@ static const char *c_escape_str(AVBPrint *dst,
> const char *src, const char sep,
> >  static const char *csv_escape_str(AVBPrint *dst, const char *src,
> const char sep, void *log_ctx)
> >  {
> >      char meta_chars[] = { sep, '"', '\n', '\r', '\0' };
> > +
> >      int needs_quoting = !!src[strcspn(src, meta_chars)];
> >
> >      if (needs_quoting)
> > @@ -117,16 +118,16 @@ typedef struct CompactContext {
> >  #undef OFFSET
> >  #define OFFSET(x) offsetof(CompactContext, x)
> >
> > -static const AVOption compact_options[]= {
> > -    {"item_sep", "set item separator",    OFFSET(item_sep_str),
> AV_OPT_TYPE_STRING, {.str="|"},  0, 0 },
> > -    {"s",        "set item separator",    OFFSET(item_sep_str),
> AV_OPT_TYPE_STRING, {.str="|"},  0, 0 },
> > -    {"nokey",    "force no key printing", OFFSET(nokey),
> AV_OPT_TYPE_BOOL,   {.i64=0},    0,        1        },
> > -    {"nk",       "force no key printing", OFFSET(nokey),
> AV_OPT_TYPE_BOOL,   {.i64=0},    0,        1        },
> > -    {"escape",   "set escape mode",       OFFSET(escape_mode_str),
> AV_OPT_TYPE_STRING, {.str="c"},  0, 0 },
> > -    {"e",        "set escape mode",       OFFSET(escape_mode_str),
> AV_OPT_TYPE_STRING, {.str="c"},  0, 0 },
> > -    {"print_section", "print section name", OFFSET(print_section),
> AV_OPT_TYPE_BOOL,   {.i64=1},    0,        1        },
> > -    {"p",             "print section name", OFFSET(print_section),
> AV_OPT_TYPE_BOOL,   {.i64=1},    0,        1        },
> > -    {NULL},
> > +static const AVOption compact_options[] = {
> > +    { "item_sep", "set item separator",      OFFSET(item_sep_str),
> AV_OPT_TYPE_STRING, { .str = "|" },  0, 0 },
> > +    { "s",        "set item separator",      OFFSET(item_sep_str),
> AV_OPT_TYPE_STRING, { .str = "|" },  0, 0 },
> > +    { "nokey",    "force no key printing",   OFFSET(nokey),
> AV_OPT_TYPE_BOOL,   { .i64 = 0   },  0, 1 },
> > +    { "nk",       "force no key printing",   OFFSET(nokey),
> AV_OPT_TYPE_BOOL,   { .i64 = 0   },  0, 1 },
> > +    { "escape",   "set escape mode",
> OFFSET(escape_mode_str), AV_OPT_TYPE_STRING, { .str = "c" },  0, 0 },
> > +    { "e",        "set escape mode",
> OFFSET(escape_mode_str), AV_OPT_TYPE_STRING, { .str = "c" },  0, 0 },
> > +    { "print_section", "print section name", OFFSET(print_section),
> AV_OPT_TYPE_BOOL,   { .i64 = 1   },  0, 1 },
> > +    { "p",             "print section name", OFFSET(print_section),
> AV_OPT_TYPE_BOOL,   { .i64 = 1   },  0, 1 },
> > +    { NULL },
> 
> About this kind of changes:
> {FIELDS...} => { FIELDS... }
> 
> I'm not against it but I wonder if there are coding style guidelines
> to follow or we end up with every single file using a different style
> - we might even enforce or automate some of them through the use of
> tooling.

I had started an attempt to create .clang-format and .clang-tidy config
files that match the formatting and warnings to the FFmpeg code base.
I got stuck with the second part after I had determined that a number of
the clang-tidy warnings are useful during work even though these are 
warning about things that are done at countless places.

But I can share the .clang-format file for review and further refinement
when there's interest. I think this would be beneficial and would allow
to achieve a higher degree of consistency.


> Rest of the changes looks good to me, thanks.


Thanks a lot for looking at it!
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".