| Message ID | AS8P250MB07444645DB3290137F52C1E08F0FA@AS8P250MB0744.EURP250.PROD.OUTLOOK.COM |
|---|---|
| State | Accepted |
| Commit | 9583a2c8ebfa39a22185171d6a4b1d6a33c044e2 |
| Headers | show |
| Series | [FFmpeg-devel] fftools/ffprobe: Use proper enum type | expand |
| Context | Check | Description |
|---|---|---|
| yinshiyou/make_loongarch64 | success | Make finished |
| yinshiyou/make_fate_loongarch64 | success | Make fate finished |
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
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(§ion->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 = §ions[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.
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(§ion->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 = §ions[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.
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(§ion->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 = §ions[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;
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(-)