> -----Original Message-----
> From: Stefano Sabatini <stefa...@gmail.com>
> Sent: Montag, 21. April 2025 18:53
> To: FFmpeg development discussions and patches <ffmpeg-
> de...@ffmpeg.org>
> Cc: softworkz <softwo...@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 <softwo...@hotmail.com>
> >
> > Signed-off-by: softworkz <softwo...@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-requ...@ffmpeg.org with subject "unsubscribe".
