[FFmpeg-devel] [PATCH] fftools/ffprobe: Use proper enum type

[FFmpeg-devel] [PATCH] fftools/ffprobe: Use proper enum type

[Andreas Rheinhardt]

This is a bit cleaner as int need not be the underlying type
of an enum if a smaller type can hold all its values.
Also declare the children_ids array as const as it never changes.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
 fftools/ffprobe.c | 38 ++++++++++++++++++--------------------
 1 file changed, 18 insertions(+), 20 deletions(-)

diff --git a/fftools/ffprobe.c b/fftools/ffprobe.c
index 5c2d4cbff1..4fcfe1164b 100644
--- a/fftools/ffprobe.c
+++ b/fftools/ffprobe.c
@@ -161,22 +161,6 @@ static int find_stream_info  = 1;
 
 #define SECTION_MAX_NB_CHILDREN 10
 
-struct section {
-    int id;             ///< unique id identifying a section
-    const char *name;
-
-#define SECTION_FLAG_IS_WRAPPER      1 ///< the section only contains other sections, but has no data at its own level
-#define SECTION_FLAG_IS_ARRAY        2 ///< the section contains an array of elements of the same type
-#define SECTION_FLAG_HAS_VARIABLE_FIELDS 4 ///< the section may contain a variable number of fields with variable keys.
-                                           ///  For these sections the element_name field is mandatory.
-    int flags;
-    int children_ids[SECTION_MAX_NB_CHILDREN+1]; ///< list of children section IDS, terminated by -1
-    const char *element_name; ///< name of the contained element, if provided
-    const char *unique_name;  ///< unique section name, in case the name is ambiguous
-    AVDictionary *entries_to_show;
-    int show_all_entries;
-};
-
 typedef enum {
     SECTION_ID_NONE = -1,
     SECTION_ID_CHAPTER,
@@ -229,6 +213,22 @@ typedef enum {
     SECTION_ID_SUBTITLE,
 } SectionID;
 
+struct section {
+    int id;             ///< unique id identifying a section
+    const char *name;
+
+#define SECTION_FLAG_IS_WRAPPER      1 ///< the section only contains other sections, but has no data at its own level
+#define SECTION_FLAG_IS_ARRAY        2 ///< the section contains an array of elements of the same type
+#define SECTION_FLAG_HAS_VARIABLE_FIELDS 4 ///< the section may contain a variable number of fields with variable keys.
+                                           ///  For these sections the element_name field is mandatory.
+    int flags;
+    const SectionID children_ids[SECTION_MAX_NB_CHILDREN+1]; ///< list of children section IDS, terminated by -1
+    const char *element_name; ///< name of the contained element, if provided
+    const char *unique_name;  ///< unique section name, in case the name is ambiguous
+    AVDictionary *entries_to_show;
+    int show_all_entries;
+};
+
 static struct section sections[] = {
     [SECTION_ID_CHAPTERS] =           { SECTION_ID_CHAPTERS, "chapters", SECTION_FLAG_IS_ARRAY, { SECTION_ID_CHAPTER, -1 } },
     [SECTION_ID_CHAPTER] =            { SECTION_ID_CHAPTER, "chapter", 0, { SECTION_ID_CHAPTER_TAGS, -1 } },
@@ -3688,8 +3688,7 @@ static inline void mark_section_show_entries(SectionID section_id,
 
     section->show_all_entries = show_all_entries;
     if (show_all_entries) {
-        SectionID *id;
-        for (id = section->children_ids; *id != -1; id++)
+        for (const SectionID *id = section->children_ids; *id != -1; id++)
             mark_section_show_entries(*id, show_all_entries, entries);
     } else {
         av_dict_copy(&section->entries_to_show, entries, 0);
@@ -4072,11 +4071,10 @@ static const OptionDef real_options[] = {
 
 static inline int check_section_show_entries(int section_id)
 {
-    int *id;
     struct section *section = &sections[section_id];
     if (sections[section_id].show_all_entries || sections[section_id].entries_to_show)
         return 1;
-    for (id = section->children_ids; *id != -1; id++)
+    for (const SectionID *id = section->children_ids; *id != -1; id++)
         if (check_section_show_entries(*id))
             return 1;
     return 0;
-- 
2.34.1

_______________________________________________
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] fftools/ffprobe: Use proper enum type

[Stefano Sabatini]

On date Sunday 2023-08-06 15:28:00 +0200, Andreas Rheinhardt wrote:
> This is a bit cleaner as int need not be the underlying type
> of an enum if a smaller type can hold all its values.
> Also declare the children_ids array as const as it never changes.
> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
> ---
>  fftools/ffprobe.c | 38 ++++++++++++++++++--------------------
>  1 file changed, 18 insertions(+), 20 deletions(-)
> 
> diff --git a/fftools/ffprobe.c b/fftools/ffprobe.c
> index 5c2d4cbff1..4fcfe1164b 100644
> --- a/fftools/ffprobe.c
> +++ b/fftools/ffprobe.c
> @@ -161,22 +161,6 @@ static int find_stream_info  = 1;
>  
>  #define SECTION_MAX_NB_CHILDREN 10
>  
> -struct section {
> -    int id;             ///< unique id identifying a section
> -    const char *name;
> -
> -#define SECTION_FLAG_IS_WRAPPER      1 ///< the section only contains other sections, but has no data at its own level
> -#define SECTION_FLAG_IS_ARRAY        2 ///< the section contains an array of elements of the same type
> -#define SECTION_FLAG_HAS_VARIABLE_FIELDS 4 ///< the section may contain a variable number of fields with variable keys.
> -                                           ///  For these sections the element_name field is mandatory.
> -    int flags;
> -    int children_ids[SECTION_MAX_NB_CHILDREN+1]; ///< list of children section IDS, terminated by -1
> -    const char *element_name; ///< name of the contained element, if provided
> -    const char *unique_name;  ///< unique section name, in case the name is ambiguous
> -    AVDictionary *entries_to_show;
> -    int show_all_entries;
> -};
> -
>  typedef enum {
>      SECTION_ID_NONE = -1,
>      SECTION_ID_CHAPTER,
> @@ -229,6 +213,22 @@ typedef enum {
>      SECTION_ID_SUBTITLE,
>  } SectionID;
>  
> +struct section {
> +    int id;             ///< unique id identifying a section
> +    const char *name;
> +
> +#define SECTION_FLAG_IS_WRAPPER      1 ///< the section only contains other sections, but has no data at its own level
> +#define SECTION_FLAG_IS_ARRAY        2 ///< the section contains an array of elements of the same type
> +#define SECTION_FLAG_HAS_VARIABLE_FIELDS 4 ///< the section may contain a variable number of fields with variable keys.
> +                                           ///  For these sections the element_name field is mandatory.
> +    int flags;
> +    const SectionID children_ids[SECTION_MAX_NB_CHILDREN+1]; ///< list of children section IDS, terminated by -1
> +    const char *element_name; ///< name of the contained element, if provided
> +    const char *unique_name;  ///< unique section name, in case the name is ambiguous
> +    AVDictionary *entries_to_show;
> +    int show_all_entries;
> +};
> +
>  static struct section sections[] = {
>      [SECTION_ID_CHAPTERS] =           { SECTION_ID_CHAPTERS, "chapters", SECTION_FLAG_IS_ARRAY, { SECTION_ID_CHAPTER, -1 } },
>      [SECTION_ID_CHAPTER] =            { SECTION_ID_CHAPTER, "chapter", 0, { SECTION_ID_CHAPTER_TAGS, -1 } },
> @@ -3688,8 +3688,7 @@ static inline void mark_section_show_entries(SectionID section_id,
>  
>      section->show_all_entries = show_all_entries;
>      if (show_all_entries) {
> -        SectionID *id;
> -        for (id = section->children_ids; *id != -1; id++)
> +        for (const SectionID *id = section->children_ids; *id != -1; id++)
>              mark_section_show_entries(*id, show_all_entries, entries);
>      } else {
>          av_dict_copy(&section->entries_to_show, entries, 0);
> @@ -4072,11 +4071,10 @@ static const OptionDef real_options[] = {
>  

>  static inline int check_section_show_entries(int section_id)
>  {
> -    int *id;
>      struct section *section = &sections[section_id];
>      if (sections[section_id].show_all_entries || sections[section_id].entries_to_show)
>          return 1;

> -    for (id = section->children_ids; *id != -1; id++)
> +    for (const SectionID *id = section->children_ids; *id != -1; id++)

I remember this might trigger a C90 style warning (mixed declarations
and statements), make sure it's not the case anymore.

>          if (check_section_show_entries(*id))
>              return 1;
>      return 0;
> -- 
> 2.34.1

LGTM, 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".

Re: [FFmpeg-devel] [PATCH] fftools/ffprobe: Use proper enum type

[Andreas Rheinhardt]

Stefano Sabatini:
> On date Sunday 2023-08-06 15:28:00 +0200, Andreas Rheinhardt wrote:
>> This is a bit cleaner as int need not be the underlying type
>> of an enum if a smaller type can hold all its values.
>> Also declare the children_ids array as const as it never changes.
>>
>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
>> ---
>>  fftools/ffprobe.c | 38 ++++++++++++++++++--------------------
>>  1 file changed, 18 insertions(+), 20 deletions(-)
>>
>> diff --git a/fftools/ffprobe.c b/fftools/ffprobe.c
>> index 5c2d4cbff1..4fcfe1164b 100644
>> --- a/fftools/ffprobe.c
>> +++ b/fftools/ffprobe.c
>> @@ -161,22 +161,6 @@ static int find_stream_info  = 1;
>>  
>>  #define SECTION_MAX_NB_CHILDREN 10
>>  
>> -struct section {
>> -    int id;             ///< unique id identifying a section
>> -    const char *name;
>> -
>> -#define SECTION_FLAG_IS_WRAPPER      1 ///< the section only contains other sections, but has no data at its own level
>> -#define SECTION_FLAG_IS_ARRAY        2 ///< the section contains an array of elements of the same type
>> -#define SECTION_FLAG_HAS_VARIABLE_FIELDS 4 ///< the section may contain a variable number of fields with variable keys.
>> -                                           ///  For these sections the element_name field is mandatory.
>> -    int flags;
>> -    int children_ids[SECTION_MAX_NB_CHILDREN+1]; ///< list of children section IDS, terminated by -1
>> -    const char *element_name; ///< name of the contained element, if provided
>> -    const char *unique_name;  ///< unique section name, in case the name is ambiguous
>> -    AVDictionary *entries_to_show;
>> -    int show_all_entries;
>> -};
>> -
>>  typedef enum {
>>      SECTION_ID_NONE = -1,
>>      SECTION_ID_CHAPTER,
>> @@ -229,6 +213,22 @@ typedef enum {
>>      SECTION_ID_SUBTITLE,
>>  } SectionID;
>>  
>> +struct section {
>> +    int id;             ///< unique id identifying a section
>> +    const char *name;
>> +
>> +#define SECTION_FLAG_IS_WRAPPER      1 ///< the section only contains other sections, but has no data at its own level
>> +#define SECTION_FLAG_IS_ARRAY        2 ///< the section contains an array of elements of the same type
>> +#define SECTION_FLAG_HAS_VARIABLE_FIELDS 4 ///< the section may contain a variable number of fields with variable keys.
>> +                                           ///  For these sections the element_name field is mandatory.
>> +    int flags;
>> +    const SectionID children_ids[SECTION_MAX_NB_CHILDREN+1]; ///< list of children section IDS, terminated by -1
>> +    const char *element_name; ///< name of the contained element, if provided
>> +    const char *unique_name;  ///< unique section name, in case the name is ambiguous
>> +    AVDictionary *entries_to_show;
>> +    int show_all_entries;
>> +};
>> +
>>  static struct section sections[] = {
>>      [SECTION_ID_CHAPTERS] =           { SECTION_ID_CHAPTERS, "chapters", SECTION_FLAG_IS_ARRAY, { SECTION_ID_CHAPTER, -1 } },
>>      [SECTION_ID_CHAPTER] =            { SECTION_ID_CHAPTER, "chapter", 0, { SECTION_ID_CHAPTER_TAGS, -1 } },
>> @@ -3688,8 +3688,7 @@ static inline void mark_section_show_entries(SectionID section_id,
>>  
>>      section->show_all_entries = show_all_entries;
>>      if (show_all_entries) {
>> -        SectionID *id;
>> -        for (id = section->children_ids; *id != -1; id++)
>> +        for (const SectionID *id = section->children_ids; *id != -1; id++)
>>              mark_section_show_entries(*id, show_all_entries, entries);
>>      } else {
>>          av_dict_copy(&section->entries_to_show, entries, 0);
>> @@ -4072,11 +4071,10 @@ static const OptionDef real_options[] = {
>>  
> 
>>  static inline int check_section_show_entries(int section_id)
>>  {
>> -    int *id;
>>      struct section *section = &sections[section_id];
>>      if (sections[section_id].show_all_entries || sections[section_id].entries_to_show)
>>          return 1;
> 
>> -    for (id = section->children_ids; *id != -1; id++)
>> +    for (const SectionID *id = section->children_ids; *id != -1; id++)
> 
> I remember this might trigger a C90 style warning (mixed declarations
> and statements), make sure it's not the case anymore.
> 

There was no warning. We completely allow declarations in for-loops (as
C99 does) and use it everywhere.

>>          if (check_section_show_entries(*id))
>>              return 1;
>>      return 0;
>> -- 
>> 2.34.1
> 
> LGTM, 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".