[FFmpeg-devel] [PATCH 1/4] fftools/textformat: narrow variable scopes

[FFmpeg-devel] [PATCH 1/4] fftools/textformat: narrow variable scopes

[Marvin Scholz]

---
 fftools/textformat/avtextformat.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/fftools/textformat/avtextformat.c b/fftools/textformat/avtextformat.c
index 14779e6f0c..f1811abb1c 100644
--- a/fftools/textformat/avtextformat.c
+++ b/fftools/textformat/avtextformat.c
@@ -101,7 +101,6 @@ static void bprint_bytes(AVBPrint *bp, const uint8_t *ubuf, size_t ubuf_size)
 int avtext_context_close(AVTextFormatContext **ptctx)
 {
     AVTextFormatContext *tctx = *ptctx;
-    int i;
     int ret = 0;
 
     if (!tctx)
@@ -117,7 +116,7 @@ int avtext_context_close(AVTextFormatContext **ptctx)
         if (tctx->formatter->priv_class)
             av_opt_free(tctx->priv);
     }
-    for (i = 0; i < SECTION_MAX_NB_LEVELS; i++)
+    for (int i = 0; i < SECTION_MAX_NB_LEVELS; i++)
         av_bprint_finalize(&tctx->section_pbuf[i], NULL);
     av_freep(&tctx->priv);
     av_opt_free(tctx);
@@ -130,7 +129,7 @@ int avtext_context_open(AVTextFormatContext **ptctx, const AVTextFormatter *form
                         const AVTextFormatSection *sections, int nb_sections, AVTextFormatOptions options, char *show_data_hash)
 {
     AVTextFormatContext *tctx;
-    int i, ret = 0;
+    int ret = 0;
 
     av_assert0(ptctx && formatter);
 
@@ -202,7 +201,7 @@ int avtext_context_open(AVTextFormatContext **ptctx, const AVTextFormatter *form
             if (ret == AVERROR(EINVAL)) {
                 const char *n;
                 av_log(NULL, AV_LOG_ERROR, "Unknown hash algorithm '%s'\nKnown algorithms:", show_data_hash);
-                for (i = 0; (n = av_hash_names(i)); i++)
+                for (unsigned i = 0; (n = av_hash_names(i)); i++)
                     av_log(NULL, AV_LOG_ERROR, " %s", n);
                 av_log(NULL, AV_LOG_ERROR, "\n");
             }
@@ -525,13 +524,13 @@ void avtext_print_data(AVTextFormatContext *tctx, const char *key,
 {
     AVBPrint bp;
     unsigned offset = 0;
-    int l, i;
+    int i;
 
     av_bprint_init(&bp, 0, AV_BPRINT_SIZE_UNLIMITED);
     av_bprintf(&bp, "\n");
     while (size) {
         av_bprintf(&bp, "%08x: ", offset);
-        l = FFMIN(size, 16);
+        int l = FFMIN(size, 16);
         for (i = 0; i < l; i++) {
             av_bprintf(&bp, "%02x", data[i]);
             if (i & 1)
@@ -571,7 +570,6 @@ void avtext_print_integers(AVTextFormatContext *tctx, const char *key,
 {
     AVBPrint bp;
     unsigned offset = 0;
-    int l, i;
 
     if (!key || !data || !format || columns <= 0 || bytes <= 0)
         return;
@@ -580,8 +578,7 @@ void avtext_print_integers(AVTextFormatContext *tctx, const char *key,
     av_bprintf(&bp, "\n");
     while (size) {
         av_bprintf(&bp, "%08x: ", offset);
-        l = FFMIN(size, columns);
-        for (i = 0; i < l; i++) {
+        for (int i = 0, l = FFMIN(size, columns); i < l; i++) {
             if      (bytes == 1) av_bprintf(&bp, format, *data);
             else if (bytes == 2) av_bprintf(&bp, format, AV_RN16(data));
             else if (bytes == 4) av_bprintf(&bp, format, AV_RN32(data));
-- 
2.39.5 (Apple Git-154)

_______________________________________________
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".

[FFmpeg-devel] [PATCH 2/4] fftools/textformat: remove noop free

[Marvin Scholz]

The tctx->hash was freed already right before.
---
 fftools/textformat/avtextformat.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/fftools/textformat/avtextformat.c b/fftools/textformat/avtextformat.c
index f1811abb1c..fa5abce261 100644
--- a/fftools/textformat/avtextformat.c
+++ b/fftools/textformat/avtextformat.c
@@ -108,8 +108,6 @@ int avtext_context_close(AVTextFormatContext **ptctx)
 
     av_hash_freep(&tctx->hash);
 
-    av_hash_freep(&tctx->hash);
-
     if (tctx->formatter) {
         if (tctx->formatter->uninit)
             ret = tctx->formatter->uninit(tctx);
-- 
2.39.5 (Apple Git-154)

_______________________________________________
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".

[FFmpeg-devel] [PATCH 3/4] fftools/textformat: do not return early

[Marvin Scholz]

This would make the goto dead code and also would not properly
call avtext_context_close.

Fix CID 1646939
---
 fftools/textformat/avtextformat.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/fftools/textformat/avtextformat.c b/fftools/textformat/avtextformat.c
index fa5abce261..e1aaa9ba57 100644
--- a/fftools/textformat/avtextformat.c
+++ b/fftools/textformat/avtextformat.c
@@ -222,7 +222,6 @@ int avtext_context_open(AVTextFormatContext **ptctx, const AVTextFormatter *form
                 av_log(tctx, AV_LOG_ERROR,
                        "Invalid UTF8 sequence %s found in string validation replace '%s'\n",
                        bp.str, tctx->string_validation_replacement);
-                return ret;
                 goto fail;
             }
         }
-- 
2.39.5 (Apple Git-154)

_______________________________________________
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".

[FFmpeg-devel] [PATCH 4/4] fftools/textformat: remove useless error check

[Marvin Scholz]

This can never be reached as the errors are already handled
locally when they occur in the code preceding this.
---
 fftools/textformat/avtextformat.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/fftools/textformat/avtextformat.c b/fftools/textformat/avtextformat.c
index e1aaa9ba57..6a309f040b 100644
--- a/fftools/textformat/avtextformat.c
+++ b/fftools/textformat/avtextformat.c
@@ -229,8 +229,6 @@ int avtext_context_open(AVTextFormatContext **ptctx, const AVTextFormatter *form
 
     if (tctx->formatter->init)
         ret = tctx->formatter->init(tctx);
-    if (ret < 0)
-        goto fail;
 
     *ptctx = tctx;
 
-- 
2.39.5 (Apple Git-154)

_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH 4/4] fftools/textformat: remove useless error check

[Andreas Rheinhardt]

Marvin Scholz:
> This can never be reached as the errors are already handled
> locally when they occur in the code preceding this.
> ---
>  fftools/textformat/avtextformat.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/fftools/textformat/avtextformat.c b/fftools/textformat/avtextformat.c
> index e1aaa9ba57..6a309f040b 100644
> --- a/fftools/textformat/avtextformat.c
> +++ b/fftools/textformat/avtextformat.c
> @@ -229,8 +229,6 @@ int avtext_context_open(AVTextFormatContext **ptctx, const AVTextFormatter *form
>  
>      if (tctx->formatter->init)
>          ret = tctx->formatter->init(tctx);
> -    if (ret < 0)
> -        goto fail;
>  
>      *ptctx = tctx;
>  

And when init fails?

- Andreas

_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH 4/4] fftools/textformat: remove useless error check

[Marvin Scholz]

On 11 Jun 2025, at 22:53, Andreas Rheinhardt wrote:

> Marvin Scholz:
>> This can never be reached as the errors are already handled
>> locally when they occur in the code preceding this.
>> ---
>>  fftools/textformat/avtextformat.c | 2 --
>>  1 file changed, 2 deletions(-)
>>
>> diff --git a/fftools/textformat/avtextformat.c b/fftools/textformat/avtextformat.c
>> index e1aaa9ba57..6a309f040b 100644
>> --- a/fftools/textformat/avtextformat.c
>> +++ b/fftools/textformat/avtextformat.c
>> @@ -229,8 +229,6 @@ int avtext_context_open(AVTextFormatContext **ptctx, const AVTextFormatter *form
>>
>>      if (tctx->formatter->init)
>>          ret = tctx->formatter->init(tctx);
>> -    if (ret < 0)
>> -        goto fail;
>>
>>      *ptctx = tctx;
>>
>
> And when init fails?

Doh indeed, I overlooked the assignment in the previous line.

Disregard this patch.

>
> - Andreas
>
> _______________________________________________
> 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".
_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH 1/4] fftools/textformat: narrow variable scopes

[softworkz .]

Hi Marvin,

> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> Marvin Scholz
> Sent: Mittwoch, 11. Juni 2025 22:42
> To: ffmpeg-devel@ffmpeg.org
> Subject: [FFmpeg-devel] [PATCH 1/4] fftools/textformat: narrow
> variable scopes
> 
> ---
>  fftools/textformat/avtextformat.c | 15 ++++++---------
>  1 file changed, 6 insertions(+), 9 deletions(-)
> 
> diff --git a/fftools/textformat/avtextformat.c
> b/fftools/textformat/avtextformat.c
> index 14779e6f0c..f1811abb1c 100644
> --- a/fftools/textformat/avtextformat.c
> +++ b/fftools/textformat/avtextformat.c
> @@ -101,7 +101,6 @@ static void bprint_bytes(AVBPrint *bp, const
> uint8_t *ubuf, size_t ubuf_size)
>  int avtext_context_close(AVTextFormatContext **ptctx)
>  {
>      AVTextFormatContext *tctx = *ptctx;
> -    int i;
>      int ret = 0;
> 
>      if (!tctx)
> @@ -117,7 +116,7 @@ int avtext_context_close(AVTextFormatContext
> **ptctx)
>          if (tctx->formatter->priv_class)
>              av_opt_free(tctx->priv);
>      }
> -    for (i = 0; i < SECTION_MAX_NB_LEVELS; i++)
> +    for (int i = 0; i < SECTION_MAX_NB_LEVELS; i++)
>          av_bprint_finalize(&tctx->section_pbuf[i], NULL);
>      av_freep(&tctx->priv);
>      av_opt_free(tctx);
> @@ -130,7 +129,7 @@ int avtext_context_open(AVTextFormatContext
> **ptctx, const AVTextFormatter *form
>                          const AVTextFormatSection *sections, int
> nb_sections, AVTextFormatOptions options, char *show_data_hash)
>  {
>      AVTextFormatContext *tctx;
> -    int i, ret = 0;
> +    int ret = 0;
> 
>      av_assert0(ptctx && formatter);
> 
> @@ -202,7 +201,7 @@ int avtext_context_open(AVTextFormatContext
> **ptctx, const AVTextFormatter *form
>              if (ret == AVERROR(EINVAL)) {
>                  const char *n;
>                  av_log(NULL, AV_LOG_ERROR, "Unknown hash algorithm
> '%s'\nKnown algorithms:", show_data_hash);
> -                for (i = 0; (n = av_hash_names(i)); i++)
> +                for (unsigned i = 0; (n = av_hash_names(i)); i++)
>                      av_log(NULL, AV_LOG_ERROR, " %s", n);
>                  av_log(NULL, AV_LOG_ERROR, "\n");
>              }
> @@ -525,13 +524,13 @@ void avtext_print_data(AVTextFormatContext
> *tctx, const char *key,
>  {
>      AVBPrint bp;
>      unsigned offset = 0;
> -    int l, i;
> +    int i;
> 
>      av_bprint_init(&bp, 0, AV_BPRINT_SIZE_UNLIMITED);
>      av_bprintf(&bp, "\n");
>      while (size) {
>          av_bprintf(&bp, "%08x: ", offset);
> -        l = FFMIN(size, 16);
> +        int l = FFMIN(size, 16);
>          for (i = 0; i < l; i++) {
>              av_bprintf(&bp, "%02x", data[i]);
>              if (i & 1)
> @@ -571,7 +570,6 @@ void avtext_print_integers(AVTextFormatContext
> *tctx, const char *key,
>  {
>      AVBPrint bp;
>      unsigned offset = 0;
> -    int l, i;
> 
>      if (!key || !data || !format || columns <= 0 || bytes <= 0)
>          return;
> @@ -580,8 +578,7 @@ void avtext_print_integers(AVTextFormatContext
> *tctx, const char *key,
>      av_bprintf(&bp, "\n");
>      while (size) {
>          av_bprintf(&bp, "%08x: ", offset);
> -        l = FFMIN(size, columns);
> -        for (i = 0; i < l; i++) {
> +        for (int i = 0, l = FFMIN(size, columns); i < l; i++) {

You are changing behavior here because size is changed inside the loop.
The pre-evaluation into l is intentional.

The remaining change should be ok.

Thanks
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".

Re: [FFmpeg-devel] [PATCH 2/4] fftools/textformat: remove noop free

[softworkz .]

> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> Marvin Scholz
> Sent: Mittwoch, 11. Juni 2025 22:42
> To: ffmpeg-devel@ffmpeg.org
> Subject: [FFmpeg-devel] [PATCH 2/4] fftools/textformat: remove noop
> free
> 
> The tctx->hash was freed already right before.
> ---
>  fftools/textformat/avtextformat.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/fftools/textformat/avtextformat.c
> b/fftools/textformat/avtextformat.c
> index f1811abb1c..fa5abce261 100644
> --- a/fftools/textformat/avtextformat.c
> +++ b/fftools/textformat/avtextformat.c
> @@ -108,8 +108,6 @@ int avtext_context_close(AVTextFormatContext
> **ptctx)
> 
>      av_hash_freep(&tctx->hash);
> 
> -    av_hash_freep(&tctx->hash);
> -
>      if (tctx->formatter) {
>          if (tctx->formatter->uninit)
>              ret = tctx->formatter->uninit(tctx);
> --
> 2.39.5 (Apple Git-154)
> 
> _______________________________________________

LGTM - I feel like I have fixed it 3 times already, but it always seemed to 
have slipped through.

Thanks a lot,
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".

Re: [FFmpeg-devel] [PATCH 3/4] fftools/textformat: do not return early

[softworkz .]

> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> Marvin Scholz
> Sent: Mittwoch, 11. Juni 2025 22:42
> To: ffmpeg-devel@ffmpeg.org
> Subject: [FFmpeg-devel] [PATCH 3/4] fftools/textformat: do not
> return early
> 
> This would make the goto dead code and also would not properly
> call avtext_context_close.
> 
> Fix CID 1646939
> ---
>  fftools/textformat/avtextformat.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/fftools/textformat/avtextformat.c
> b/fftools/textformat/avtextformat.c
> index fa5abce261..e1aaa9ba57 100644
> --- a/fftools/textformat/avtextformat.c
> +++ b/fftools/textformat/avtextformat.c
> @@ -222,7 +222,6 @@ int avtext_context_open(AVTextFormatContext
> **ptctx, const AVTextFormatter *form
>                  av_log(tctx, AV_LOG_ERROR,
>                         "Invalid UTF8 sequence %s found in string
> validation replace '%s'\n",
>                         bp.str, tctx-
> >string_validation_replacement);
> -                return ret;
>                  goto fail;
>              }
>          }
> --
> 2.39.5 (Apple Git-154)
> 
> _______________________________________________

LGTM, thank you!

Best regards
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".

Re: [FFmpeg-devel] [PATCH 1/4] fftools/textformat: narrow variable scopes

[Marvin Scholz]

On 12 Jun 2025, at 4:26, softworkz . wrote:

> Hi Marvin,
>
>> -----Original Message-----
>> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
>> Marvin Scholz
>> Sent: Mittwoch, 11. Juni 2025 22:42
>> To: ffmpeg-devel@ffmpeg.org
>> Subject: [FFmpeg-devel] [PATCH 1/4] fftools/textformat: narrow
>> variable scopes
>>
>> ---
>>  fftools/textformat/avtextformat.c | 15 ++++++---------
>>  1 file changed, 6 insertions(+), 9 deletions(-)
>>
>> diff --git a/fftools/textformat/avtextformat.c
>> b/fftools/textformat/avtextformat.c
>> index 14779e6f0c..f1811abb1c 100644
>> --- a/fftools/textformat/avtextformat.c
>> +++ b/fftools/textformat/avtextformat.c
>> @@ -101,7 +101,6 @@ static void bprint_bytes(AVBPrint *bp, const
>> uint8_t *ubuf, size_t ubuf_size)
>>  int avtext_context_close(AVTextFormatContext **ptctx)
>>  {
>>      AVTextFormatContext *tctx = *ptctx;
>> -    int i;
>>      int ret = 0;
>>
>>      if (!tctx)
>> @@ -117,7 +116,7 @@ int avtext_context_close(AVTextFormatContext
>> **ptctx)
>>          if (tctx->formatter->priv_class)
>>              av_opt_free(tctx->priv);
>>      }
>> -    for (i = 0; i < SECTION_MAX_NB_LEVELS; i++)
>> +    for (int i = 0; i < SECTION_MAX_NB_LEVELS; i++)
>>          av_bprint_finalize(&tctx->section_pbuf[i], NULL);
>>      av_freep(&tctx->priv);
>>      av_opt_free(tctx);
>> @@ -130,7 +129,7 @@ int avtext_context_open(AVTextFormatContext
>> **ptctx, const AVTextFormatter *form
>>                          const AVTextFormatSection *sections, int
>> nb_sections, AVTextFormatOptions options, char *show_data_hash)
>>  {
>>      AVTextFormatContext *tctx;
>> -    int i, ret = 0;
>> +    int ret = 0;
>>
>>      av_assert0(ptctx && formatter);
>>
>> @@ -202,7 +201,7 @@ int avtext_context_open(AVTextFormatContext
>> **ptctx, const AVTextFormatter *form
>>              if (ret == AVERROR(EINVAL)) {
>>                  const char *n;
>>                  av_log(NULL, AV_LOG_ERROR, "Unknown hash algorithm
>> '%s'\nKnown algorithms:", show_data_hash);
>> -                for (i = 0; (n = av_hash_names(i)); i++)
>> +                for (unsigned i = 0; (n = av_hash_names(i)); i++)
>>                      av_log(NULL, AV_LOG_ERROR, " %s", n);
>>                  av_log(NULL, AV_LOG_ERROR, "\n");
>>              }
>> @@ -525,13 +524,13 @@ void avtext_print_data(AVTextFormatContext
>> *tctx, const char *key,
>>  {
>>      AVBPrint bp;
>>      unsigned offset = 0;
>> -    int l, i;
>> +    int i;
>>
>>      av_bprint_init(&bp, 0, AV_BPRINT_SIZE_UNLIMITED);
>>      av_bprintf(&bp, "\n");
>>      while (size) {
>>          av_bprintf(&bp, "%08x: ", offset);
>> -        l = FFMIN(size, 16);
>> +        int l = FFMIN(size, 16);
>>          for (i = 0; i < l; i++) {
>>              av_bprintf(&bp, "%02x", data[i]);
>>              if (i & 1)
>> @@ -571,7 +570,6 @@ void avtext_print_integers(AVTextFormatContext
>> *tctx, const char *key,
>>  {
>>      AVBPrint bp;
>>      unsigned offset = 0;
>> -    int l, i;
>>
>>      if (!key || !data || !format || columns <= 0 || bytes <= 0)
>>          return;
>> @@ -580,8 +578,7 @@ void avtext_print_integers(AVTextFormatContext
>> *tctx, const char *key,
>>      av_bprintf(&bp, "\n");
>>      while (size) {
>>          av_bprintf(&bp, "%08x: ", offset);
>> -        l = FFMIN(size, columns);
>> -        for (i = 0; i < l; i++) {
>> +        for (int i = 0, l = FFMIN(size, columns); i < l; i++) {
>
> You are changing behavior here because size is changed inside the loop.
> The pre-evaluation into l is intentional.
>

I fail to see how I change anything here though? It is still just assigned before
the loop runs, just that l is scoped narrower?

> The remaining change should be ok.
>
> Thanks
> 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".
_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH 1/4] fftools/textformat: narrow variable scopes

[softworkz .]

> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> Marvin Scholz
> Sent: Donnerstag, 12. Juni 2025 13:25
> To: FFmpeg development discussions and patches <ffmpeg-
> devel@ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH 1/4] fftools/textformat: narrow
> variable scopes
> 
> 
> 
> On 12 Jun 2025, at 4:26, softworkz . wrote:
> 
> > Hi Marvin,
> >
> >> -----Original Message-----
> >> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf
> Of
> >> Marvin Scholz
> >> Sent: Mittwoch, 11. Juni 2025 22:42
> >> To: ffmpeg-devel@ffmpeg.org
> >> Subject: [FFmpeg-devel] [PATCH 1/4] fftools/textformat: narrow
> >> variable scopes
> >>
> >> ---
> >>  fftools/textformat/avtextformat.c | 15 ++++++---------
> >>  1 file changed, 6 insertions(+), 9 deletions(-)
> >>
> >> diff --git a/fftools/textformat/avtextformat.c
> >> b/fftools/textformat/avtextformat.c
> >> index 14779e6f0c..f1811abb1c 100644
> >> --- a/fftools/textformat/avtextformat.c
> >> +++ b/fftools/textformat/avtextformat.c
> >> @@ -101,7 +101,6 @@ static void bprint_bytes(AVBPrint *bp, const
> >> uint8_t *ubuf, size_t ubuf_size)
> >>  int avtext_context_close(AVTextFormatContext **ptctx)
> >>  {
> >>      AVTextFormatContext *tctx = *ptctx;
> >> -    int i;
> >>      int ret = 0;
> >>
> >>      if (!tctx)
> >> @@ -117,7 +116,7 @@ int avtext_context_close(AVTextFormatContext
> >> **ptctx)
> >>          if (tctx->formatter->priv_class)
> >>              av_opt_free(tctx->priv);
> >>      }
> >> -    for (i = 0; i < SECTION_MAX_NB_LEVELS; i++)
> >> +    for (int i = 0; i < SECTION_MAX_NB_LEVELS; i++)
> >>          av_bprint_finalize(&tctx->section_pbuf[i], NULL);
> >>      av_freep(&tctx->priv);
> >>      av_opt_free(tctx);
> >> @@ -130,7 +129,7 @@ int avtext_context_open(AVTextFormatContext
> >> **ptctx, const AVTextFormatter *form
> >>                          const AVTextFormatSection *sections,
> int
> >> nb_sections, AVTextFormatOptions options, char *show_data_hash)
> >>  {
> >>      AVTextFormatContext *tctx;
> >> -    int i, ret = 0;
> >> +    int ret = 0;
> >>
> >>      av_assert0(ptctx && formatter);
> >>
> >> @@ -202,7 +201,7 @@ int avtext_context_open(AVTextFormatContext
> >> **ptctx, const AVTextFormatter *form
> >>              if (ret == AVERROR(EINVAL)) {
> >>                  const char *n;
> >>                  av_log(NULL, AV_LOG_ERROR, "Unknown hash
> algorithm
> >> '%s'\nKnown algorithms:", show_data_hash);
> >> -                for (i = 0; (n = av_hash_names(i)); i++)
> >> +                for (unsigned i = 0; (n = av_hash_names(i));
> i++)
> >>                      av_log(NULL, AV_LOG_ERROR, " %s", n);
> >>                  av_log(NULL, AV_LOG_ERROR, "\n");
> >>              }
> >> @@ -525,13 +524,13 @@ void avtext_print_data(AVTextFormatContext
> >> *tctx, const char *key,
> >>  {
> >>      AVBPrint bp;
> >>      unsigned offset = 0;
> >> -    int l, i;
> >> +    int i;
> >>
> >>      av_bprint_init(&bp, 0, AV_BPRINT_SIZE_UNLIMITED);
> >>      av_bprintf(&bp, "\n");
> >>      while (size) {
> >>          av_bprintf(&bp, "%08x: ", offset);
> >> -        l = FFMIN(size, 16);
> >> +        int l = FFMIN(size, 16);
> >>          for (i = 0; i < l; i++) {
> >>              av_bprintf(&bp, "%02x", data[i]);
> >>              if (i & 1)
> >> @@ -571,7 +570,6 @@ void
> avtext_print_integers(AVTextFormatContext
> >> *tctx, const char *key,
> >>  {
> >>      AVBPrint bp;
> >>      unsigned offset = 0;
> >> -    int l, i;
> >>
> >>      if (!key || !data || !format || columns <= 0 || bytes <= 0)
> >>          return;
> >> @@ -580,8 +578,7 @@ void
> avtext_print_integers(AVTextFormatContext
> >> *tctx, const char *key,
> >>      av_bprintf(&bp, "\n");
> >>      while (size) {
> >>          av_bprintf(&bp, "%08x: ", offset);
> >> -        l = FFMIN(size, columns);
> >> -        for (i = 0; i < l; i++) {
> >> +        for (int i = 0, l = FFMIN(size, columns); i < l; i++) {
> >
> > You are changing behavior here because size is changed inside the
> loop.
> > The pre-evaluation into l is intentional.
> >
> 
> I fail to see how I change anything here though? It is still just
> assigned before
> the loop runs, just that l is scoped narrower?

This is the original code:

        l = FFMIN(size, columns);
        for (i = 0; i < l; i++) {
            if      (bytes == 1) av_bprintf(&bp, format, *data);
            else if (bytes == 2) av_bprintf(&bp, format, AV_RN16(data));
            else if (bytes == 4) av_bprintf(&bp, format, AV_RN32(data));
            data += bytes;
            size--;
        }

See the size-- ?


Thanks
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".

Re: [FFmpeg-devel] [PATCH 1/4] fftools/textformat: narrow variable scopes

[Marvin Scholz]

On 12 Jun 2025, at 18:10, softworkz . wrote:

>> -----Original Message-----
>> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
>> Marvin Scholz
>> Sent: Donnerstag, 12. Juni 2025 13:25
>> To: FFmpeg development discussions and patches <ffmpeg-
>> devel@ffmpeg.org>
>> Subject: Re: [FFmpeg-devel] [PATCH 1/4] fftools/textformat: narrow
>> variable scopes
>>
>>
>>
>> On 12 Jun 2025, at 4:26, softworkz . wrote:
>>
>>> Hi Marvin,
>>>
>>>> -----Original Message-----
>>>> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf
>> Of
>>>> Marvin Scholz
>>>> Sent: Mittwoch, 11. Juni 2025 22:42
>>>> To: ffmpeg-devel@ffmpeg.org
>>>> Subject: [FFmpeg-devel] [PATCH 1/4] fftools/textformat: narrow
>>>> variable scopes
>>>>
>>>> ---
>>>>  fftools/textformat/avtextformat.c | 15 ++++++---------
>>>>  1 file changed, 6 insertions(+), 9 deletions(-)
>>>>
>>>> diff --git a/fftools/textformat/avtextformat.c
>>>> b/fftools/textformat/avtextformat.c
>>>> index 14779e6f0c..f1811abb1c 100644
>>>> --- a/fftools/textformat/avtextformat.c
>>>> +++ b/fftools/textformat/avtextformat.c
>>>> @@ -101,7 +101,6 @@ static void bprint_bytes(AVBPrint *bp, const
>>>> uint8_t *ubuf, size_t ubuf_size)
>>>>  int avtext_context_close(AVTextFormatContext **ptctx)
>>>>  {
>>>>      AVTextFormatContext *tctx = *ptctx;
>>>> -    int i;
>>>>      int ret = 0;
>>>>
>>>>      if (!tctx)
>>>> @@ -117,7 +116,7 @@ int avtext_context_close(AVTextFormatContext
>>>> **ptctx)
>>>>          if (tctx->formatter->priv_class)
>>>>              av_opt_free(tctx->priv);
>>>>      }
>>>> -    for (i = 0; i < SECTION_MAX_NB_LEVELS; i++)
>>>> +    for (int i = 0; i < SECTION_MAX_NB_LEVELS; i++)
>>>>          av_bprint_finalize(&tctx->section_pbuf[i], NULL);
>>>>      av_freep(&tctx->priv);
>>>>      av_opt_free(tctx);
>>>> @@ -130,7 +129,7 @@ int avtext_context_open(AVTextFormatContext
>>>> **ptctx, const AVTextFormatter *form
>>>>                          const AVTextFormatSection *sections,
>> int
>>>> nb_sections, AVTextFormatOptions options, char *show_data_hash)
>>>>  {
>>>>      AVTextFormatContext *tctx;
>>>> -    int i, ret = 0;
>>>> +    int ret = 0;
>>>>
>>>>      av_assert0(ptctx && formatter);
>>>>
>>>> @@ -202,7 +201,7 @@ int avtext_context_open(AVTextFormatContext
>>>> **ptctx, const AVTextFormatter *form
>>>>              if (ret == AVERROR(EINVAL)) {
>>>>                  const char *n;
>>>>                  av_log(NULL, AV_LOG_ERROR, "Unknown hash
>> algorithm
>>>> '%s'\nKnown algorithms:", show_data_hash);
>>>> -                for (i = 0; (n = av_hash_names(i)); i++)
>>>> +                for (unsigned i = 0; (n = av_hash_names(i));
>> i++)
>>>>                      av_log(NULL, AV_LOG_ERROR, " %s", n);
>>>>                  av_log(NULL, AV_LOG_ERROR, "\n");
>>>>              }
>>>> @@ -525,13 +524,13 @@ void avtext_print_data(AVTextFormatContext
>>>> *tctx, const char *key,
>>>>  {
>>>>      AVBPrint bp;
>>>>      unsigned offset = 0;
>>>> -    int l, i;
>>>> +    int i;
>>>>
>>>>      av_bprint_init(&bp, 0, AV_BPRINT_SIZE_UNLIMITED);
>>>>      av_bprintf(&bp, "\n");
>>>>      while (size) {
>>>>          av_bprintf(&bp, "%08x: ", offset);
>>>> -        l = FFMIN(size, 16);
>>>> +        int l = FFMIN(size, 16);
>>>>          for (i = 0; i < l; i++) {
>>>>              av_bprintf(&bp, "%02x", data[i]);
>>>>              if (i & 1)
>>>> @@ -571,7 +570,6 @@ void
>> avtext_print_integers(AVTextFormatContext
>>>> *tctx, const char *key,
>>>>  {
>>>>      AVBPrint bp;
>>>>      unsigned offset = 0;
>>>> -    int l, i;
>>>>
>>>>      if (!key || !data || !format || columns <= 0 || bytes <= 0)
>>>>          return;
>>>> @@ -580,8 +578,7 @@ void
>> avtext_print_integers(AVTextFormatContext
>>>> *tctx, const char *key,
>>>>      av_bprintf(&bp, "\n");
>>>>      while (size) {
>>>>          av_bprintf(&bp, "%08x: ", offset);
>>>> -        l = FFMIN(size, columns);
>>>> -        for (i = 0; i < l; i++) {
>>>> +        for (int i = 0, l = FFMIN(size, columns); i < l; i++) {
>>>
>>> You are changing behavior here because size is changed inside the
>> loop.
>>> The pre-evaluation into l is intentional.
>>>
>>
>> I fail to see how I change anything here though? It is still just
>> assigned before
>> the loop runs, just that l is scoped narrower?
>
> This is the original code:
>
>         l = FFMIN(size, columns);
>         for (i = 0; i < l; i++) {
>             if      (bytes == 1) av_bprintf(&bp, format, *data);
>             else if (bytes == 2) av_bprintf(&bp, format, AV_RN16(data));
>             else if (bytes == 4) av_bprintf(&bp, format, AV_RN32(data));
>             data += bytes;
>             size--;
>         }
>
> See the size-- ?
>

I see that but again I fail to see how this changes anything.
I am not changing that l is assigned before the loop runs, just
narrow the scope to the loop.

See: https://godbolt.org/z/c3hGKfne4

>
> Thanks
> 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".
_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH 1/4] fftools/textformat: narrow variable scopes

[softworkz .]

> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> Marvin Scholz
> Sent: Donnerstag, 12. Juni 2025 19:07
> To: FFmpeg development discussions and patches <ffmpeg-
> devel@ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH 1/4] fftools/textformat: narrow
> variable scopes
> 
> 
> 
> On 12 Jun 2025, at 18:10, softworkz . wrote:
> 
> >> -----Original Message-----
> >> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf
> Of
> >> Marvin Scholz
> >> Sent: Donnerstag, 12. Juni 2025 13:25
> >> To: FFmpeg development discussions and patches <ffmpeg-
> >> devel@ffmpeg.org>
> >> Subject: Re: [FFmpeg-devel] [PATCH 1/4] fftools/textformat:
> narrow
> >> variable scopes
> >>
> >>
> >>
> >> On 12 Jun 2025, at 4:26, softworkz . wrote:
> >>
> >>> Hi Marvin,
> >>>
> >>>> -----Original Message-----
> >>>> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf
> >> Of
> >>>> Marvin Scholz
> >>>> Sent: Mittwoch, 11. Juni 2025 22:42
> >>>> To: ffmpeg-devel@ffmpeg.org
> >>>> Subject: [FFmpeg-devel] [PATCH 1/4] fftools/textformat: narrow
> >>>> variable scopes
> >>>>
> >>>> ---
> >>>>  fftools/textformat/avtextformat.c | 15 ++++++---------
> >>>>  1 file changed, 6 insertions(+), 9 deletions(-)
> >>>>
> >>>> diff --git a/fftools/textformat/avtextformat.c
> >>>> b/fftools/textformat/avtextformat.c
> >>>> index 14779e6f0c..f1811abb1c 100644
> >>>> --- a/fftools/textformat/avtextformat.c
> >>>> +++ b/fftools/textformat/avtextformat.c
> >>>> @@ -101,7 +101,6 @@ static void bprint_bytes(AVBPrint *bp,
> const
> >>>> uint8_t *ubuf, size_t ubuf_size)
> >>>>  int avtext_context_close(AVTextFormatContext **ptctx)
> >>>>  {
> >>>>      AVTextFormatContext *tctx = *ptctx;
> >>>> -    int i;
> >>>>      int ret = 0;
> >>>>
> >>>>      if (!tctx)
> >>>> @@ -117,7 +116,7 @@ int
> avtext_context_close(AVTextFormatContext
> >>>> **ptctx)
> >>>>          if (tctx->formatter->priv_class)
> >>>>              av_opt_free(tctx->priv);
> >>>>      }
> >>>> -    for (i = 0; i < SECTION_MAX_NB_LEVELS; i++)
> >>>> +    for (int i = 0; i < SECTION_MAX_NB_LEVELS; i++)
> >>>>          av_bprint_finalize(&tctx->section_pbuf[i], NULL);
> >>>>      av_freep(&tctx->priv);
> >>>>      av_opt_free(tctx);
> >>>> @@ -130,7 +129,7 @@ int
> avtext_context_open(AVTextFormatContext
> >>>> **ptctx, const AVTextFormatter *form
> >>>>                          const AVTextFormatSection *sections,
> >> int
> >>>> nb_sections, AVTextFormatOptions options, char
> *show_data_hash)
> >>>>  {
> >>>>      AVTextFormatContext *tctx;
> >>>> -    int i, ret = 0;
> >>>> +    int ret = 0;
> >>>>
> >>>>      av_assert0(ptctx && formatter);
> >>>>
> >>>> @@ -202,7 +201,7 @@ int
> avtext_context_open(AVTextFormatContext
> >>>> **ptctx, const AVTextFormatter *form
> >>>>              if (ret == AVERROR(EINVAL)) {
> >>>>                  const char *n;
> >>>>                  av_log(NULL, AV_LOG_ERROR, "Unknown hash
> >> algorithm
> >>>> '%s'\nKnown algorithms:", show_data_hash);
> >>>> -                for (i = 0; (n = av_hash_names(i)); i++)
> >>>> +                for (unsigned i = 0; (n = av_hash_names(i));
> >> i++)
> >>>>                      av_log(NULL, AV_LOG_ERROR, " %s", n);
> >>>>                  av_log(NULL, AV_LOG_ERROR, "\n");
> >>>>              }
> >>>> @@ -525,13 +524,13 @@ void
> avtext_print_data(AVTextFormatContext
> >>>> *tctx, const char *key,
> >>>>  {
> >>>>      AVBPrint bp;
> >>>>      unsigned offset = 0;
> >>>> -    int l, i;
> >>>> +    int i;
> >>>>
> >>>>      av_bprint_init(&bp, 0, AV_BPRINT_SIZE_UNLIMITED);
> >>>>      av_bprintf(&bp, "\n");
> >>>>      while (size) {
> >>>>          av_bprintf(&bp, "%08x: ", offset);
> >>>> -        l = FFMIN(size, 16);
> >>>> +        int l = FFMIN(size, 16);
> >>>>          for (i = 0; i < l; i++) {
> >>>>              av_bprintf(&bp, "%02x", data[i]);
> >>>>              if (i & 1)
> >>>> @@ -571,7 +570,6 @@ void
> >> avtext_print_integers(AVTextFormatContext
> >>>> *tctx, const char *key,
> >>>>  {
> >>>>      AVBPrint bp;
> >>>>      unsigned offset = 0;
> >>>> -    int l, i;
> >>>>
> >>>>      if (!key || !data || !format || columns <= 0 || bytes <=
> 0)
> >>>>          return;
> >>>> @@ -580,8 +578,7 @@ void
> >> avtext_print_integers(AVTextFormatContext
> >>>> *tctx, const char *key,
> >>>>      av_bprintf(&bp, "\n");
> >>>>      while (size) {
> >>>>          av_bprintf(&bp, "%08x: ", offset);
> >>>> -        l = FFMIN(size, columns);
> >>>> -        for (i = 0; i < l; i++) {
> >>>> +        for (int i = 0, l = FFMIN(size, columns); i < l; i++)
> {
> >>>
> >>> You are changing behavior here because size is changed inside
> the
> >> loop.
> >>> The pre-evaluation into l is intentional.
> >>>
> >>
> >> I fail to see how I change anything here though? It is still
> just
> >> assigned before
> >> the loop runs, just that l is scoped narrower?
> >
> > This is the original code:
> >
> >         l = FFMIN(size, columns);
> >         for (i = 0; i < l; i++) {
> >             if      (bytes == 1) av_bprintf(&bp, format, *data);
> >             else if (bytes == 2) av_bprintf(&bp, format,
> AV_RN16(data));
> >             else if (bytes == 4) av_bprintf(&bp, format,
> AV_RN32(data));
> >             data += bytes;
> >             size--;
> >         }
> >
> > See the size-- ?
> >
> 
> I see that but again I fail to see how this changes anything.
> I am not changing that l is assigned before the loop runs, just
> narrow the scope to the loop.
> 
> See: https://godbolt.org/z/c3hGKfne4

Ooops - my fault. I didn't look carefully enough.

LGTM! 

Thanks and sorry,
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".