From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <ffmpeg-devel-bounces@ffmpeg.org>
Received: from ffbox0-bg.mplayerhq.hu (ffbox0-bg.ffmpeg.org [79.124.17.100])
by master.gitmailbox.com (Postfix) with ESMTPS id 0F61B4D9AA
for <ffmpegdev@gitmailbox.com>; Mon, 21 Apr 2025 16:52:58 +0000 (UTC)
Received: from [127.0.1.1] (localhost [127.0.0.1])
by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id A3DF7687DA2;
Mon, 21 Apr 2025 19:52:53 +0300 (EEST)
Received: from mail-ej1-f53.google.com (mail-ej1-f53.google.com
[209.85.218.53])
by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id DE182687BFE
for <ffmpeg-devel@ffmpeg.org>; Mon, 21 Apr 2025 19:52:46 +0300 (EEST)
Received: by mail-ej1-f53.google.com with SMTP id
a640c23a62f3a-ac29fd22163so638947566b.3
for <ffmpeg-devel@ffmpeg.org>; Mon, 21 Apr 2025 09:52:46 -0700 (PDT)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed;
d=gmail.com; s=20230601; t=1745254366; x=1745859166; darn=ffmpeg.org;
h=user-agent:in-reply-to:content-disposition:mime-version:references
:mail-followup-to:message-id:subject:cc:to:from:date:from:to:cc
:subject:date:message-id:reply-to;
bh=ixxUP+mgMc0sRTeEnX2rTT23bjakYRvL2CQPXQiP6Q4=;
b=ON/T0sBpzy12MQKmZiV8eHwAsgeSjE5UTEmbVObyrNkbplC9a5XEjeXJaASPTITrbg
x7UvxjdwVtyvdWbylXf8LJlCnvl1F7ai/UGgpXRf5Vc/ejL34pxz3iHNg/MjL6r8NenQ
DHaGeMlCMAInSq1kZWnn3tAuQzWvJF6DV2WlkxaxciYaukNlpSHe9s+ackOGUrg29Z04
XDz8oA7pv8ZcOX3QZ9kjqfWNisoQgTbxIkVtGA0FvxLP4vNV6pzFvk+waG/5gtJQuM9s
+Id3urSEGVP8S2Dp+aR7qqm4BuTr23/4q2I+1D/1JvMXyo5TL4taCshpwy6O/9Q5N0li
LWXg==
X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed;
d=1e100.net; s=20230601; t=1745254366; x=1745859166;
h=user-agent:in-reply-to:content-disposition:mime-version:references
:mail-followup-to:message-id:subject:cc:to:from:date
:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to;
bh=ixxUP+mgMc0sRTeEnX2rTT23bjakYRvL2CQPXQiP6Q4=;
b=IexrLhhfTydmCWyK7326ch37O26cPLlgfK0gDN9gIz+WakqCBAstyUs9yCX5w0Ulcu
gpCEmpS1vM2CC4koANFEvom5g5juQjk6zm1eaJEdMe8/D9encL3UNonzZy9l2rf4qR0U
sQ1CBKBXWzH+O1EQXMRrZD9m4EeWfr6Tv4kmrlO/tDXsy/QDK/SeRyn398YdHnp8+dnm
Nk6kUs+kP/EsbW9sTI5sfWFOSku0ylLYtQ+v8PFpUcXuLCqTWk3DyjBQmOdh98QZPax7
ZRFDFTSEvSsxZ9ZeZkByqhVfQe9nzkN1NJOE8ykD6NJk87b3Q9a+aiLQV0CZsQcjpgd2
pcnQ==
X-Gm-Message-State: AOJu0YzTD006p/zxJFPRjyTzq7MugXj6PJcOgz4RF7F525FUgIxJtv2Q
Mjc7VnfaWVaKHlrGa7luVzzVBVsQhbizjFmD2IwVmLMmc7lUlSyov3MEGw==
X-Gm-Gg: ASbGnctVqEXq6egah22w4t5K1zzlGYiBZmoz+W9NgpXmaoUQD3daiQMp5sLwnS8+L0b
/w0iQhBbjTXXAd16+2ja5ttfOgeGdELxqh8nt/6dDJUekGLoiXCpjmjIEvmSY4OVybpEO/lVwvi
POJfW3kuXSVoNjxU5HrpLNQDXTSbW6fUnFhBah7naHW3EZDQS0akKQtz+6FgjmsoFMa6GERuPHj
kMO+bcu4nqH9sloeMpFVT1T3LhI4SOGZzRynmE3UBu53OTtqBvBkSQlJkkg49+FY5B9CLq/Gv1R
ICz07YE4IjF04ISwyfdMIaluHqYNWzutVAMFlhvmtJXNjoLZPLlKMCZ2+8BTwP1rkXKEBiC//mc
LuBQH
X-Google-Smtp-Source: AGHT+IGjhx8/hEqbz9+xMTB5xIGIh2D7b9YfsOeHG2t2PpddHtH6/ynXVaLKGrdhZXAakesEQaKKig==
X-Received: by 2002:a17:907:3cd5:b0:ac7:b368:b193 with SMTP id
a640c23a62f3a-acb74b7e64bmr1064945266b.27.1745254365556;
Mon, 21 Apr 2025 09:52:45 -0700 (PDT)
Received: from mariano (dynamic-adsl-84-220-189-10.clienti.tiscali.it.
[84.220.189.10]) by smtp.gmail.com with ESMTPSA id
a640c23a62f3a-acb6ec0c0e6sm528725166b.26.2025.04.21.09.52.44
(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);
Mon, 21 Apr 2025 09:52:45 -0700 (PDT)
Received: by mariano (Postfix, from userid 1000)
id 7B2FFBFCE8; Mon, 21 Apr 2025 18:52:43 +0200 (CEST)
Date: Mon, 21 Apr 2025 18:52:43 +0200
From: Stefano Sabatini <stefasab@gmail.com>
To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org>
Message-ID: <aAZ32+EdhsWEuU9b@mariano>
Mail-Followup-To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org>,
softworkz <softworkz-at-hotmail.com@ffmpeg.org>
References: <pull.66.ffstaging.FFmpeg.1744634826.ffmpegagent@gmail.com>
<3df2018c81327379621cbe70c92d51b30cf9f81f.1744634826.git.ffmpegagent@gmail.com>
MIME-Version: 1.0
Content-Disposition: inline
In-Reply-To: <3df2018c81327379621cbe70c92d51b30cf9f81f.1744634826.git.ffmpegagent@gmail.com>
User-Agent: Mutt/2.1.4 (2021-12-11)
Subject: Re: [FFmpeg-devel] [PATCH 1/9] fftools/textformat: Formatting and
whitespace changes
X-BeenThere: ffmpeg-devel@ffmpeg.org
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: FFmpeg development discussions and patches <ffmpeg-devel.ffmpeg.org>
List-Unsubscribe: <https://ffmpeg.org/mailman/options/ffmpeg-devel>,
<mailto:ffmpeg-devel-request@ffmpeg.org?subject=unsubscribe>
List-Archive: <https://ffmpeg.org/pipermail/ffmpeg-devel>
List-Post: <mailto:ffmpeg-devel@ffmpeg.org>
List-Help: <mailto:ffmpeg-devel-request@ffmpeg.org?subject=help>
List-Subscribe: <https://ffmpeg.org/mailman/listinfo/ffmpeg-devel>,
<mailto:ffmpeg-devel-request@ffmpeg.org?subject=subscribe>
Reply-To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org>
Cc: softworkz <softworkz@hotmail.com>
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Errors-To: ffmpeg-devel-bounces@ffmpeg.org
Sender: "ffmpeg-devel" <ffmpeg-devel-bounces@ffmpeg.org>
Archived-At: <https://master.gitmailbox.com/ffmpegdev/aAZ32+EdhsWEuU9b@mariano/>
List-Archive: <https://master.gitmailbox.com/ffmpegdev/>
List-Post: <mailto:ffmpegdev@gitmailbox.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".