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

[Stefano Sabatini <stefasab@gmail.com>]

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.

>  {
>      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).
  
[...]
> @@ -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.

[...]

Rest of the changes looks good to me, thanks.
_______________________________________________
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".