| Message ID | 20240416202841.725706-4-aalbersh@redhat.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | Refactoring from Coverity scan fixes | expand |
On Tue, Apr 16, 2024 at 10:28:42PM +0200, Andrey Albershteyn wrote: > Make flist_find_type() more readable by unloading field type > matching to the helper. > > Signed-off-by: Andrey Albershteyn <aalbersh@redhat.com> > --- > db/flist.c | 59 ++++++++++++++++++++++++++++++++++-------------------- > 1 file changed, 37 insertions(+), 22 deletions(-) > > diff --git a/db/flist.c b/db/flist.c > index 0a6cc5fcee43..18052a744a65 100644 > --- a/db/flist.c > +++ b/db/flist.c > @@ -400,6 +400,40 @@ flist_split( > return v; > } > > +flist_t * > +flist_field_match( Is this function going to be used outside of this module, or should it be declared static? > + const field_t *field, > + fldt_t type, > + void *obj, > + int startoff) > +{ > + flist_t *fl; > + int count; > + const ftattr_t *fa; > + > + fl = flist_make(field->name); > + fl->fld = field; > + if (field->ftyp == type) > + return fl; > + count = fcount(field, obj, startoff); > + if (!count) > + goto out; > + fa = &ftattrtab[field->ftyp]; > + if (fa->subfld) { You could do: if (!fa->subfld) goto out; nfl = flist_find_ftyp(...); to reduce the indenting here. But I think you were trying to make before and after as identical as possible, right? > + flist_t *nfl; > + > + nfl = flist_find_ftyp(fa->subfld, type, obj, startoff); > + if (nfl) { > + fl->child = nfl; > + return fl; > + } > + } > + > +out: > + flist_free(fl); > + return NULL; > +} > + > /* > * Given a set of fields, scan for a field of the given type. > * Return an flist leading to the first found field > @@ -413,33 +447,14 @@ flist_find_ftyp( > void *obj, > int startoff) > { > - flist_t *fl; > const field_t *f; > - int count; > - const ftattr_t *fa; > + flist_t *fl; > > for (f = fields; f->name; f++) { > - fl = flist_make(f->name); > - fl->fld = f; > - if (f->ftyp == type) > + if ((fl = flist_field_match(f, type, obj, startoff)) != NULL) > return fl; Normally these days we expand assign-and-check when not in a loop control test: fl = flist_field_match(...); if (fl) return fl; But those last two comments merely reflect my style preference; the question I care most about is the one about the static attribute. --D > - count = fcount(f, obj, startoff); > - if (!count) { > - flist_free(fl); > - continue; > - } > - fa = &ftattrtab[f->ftyp]; > - if (fa->subfld) { > - flist_t *nfl; > - > - nfl = flist_find_ftyp(fa->subfld, type, obj, startoff); > - if (nfl) { > - fl->child = nfl; > - return fl; > - } > - } > - flist_free(fl); > } > + > return NULL; > } > > -- > 2.42.0 > >
On Tue, Apr 16, 2024 at 01:53:48PM -0700, Darrick J. Wong wrote: > You could do: > > if (!fa->subfld) > goto out; > > nfl = flist_find_ftyp(...); > > to reduce the indenting here. But I think you were trying to make > before and after as identical as possible, right? FYI, the goto version s what I had in mind with my original suggestion, although the version here shoud be ok as well. We'll need the static either way, though.
diff --git a/db/flist.c b/db/flist.c index 0a6cc5fcee43..18052a744a65 100644 --- a/db/flist.c +++ b/db/flist.c @@ -400,6 +400,40 @@ flist_split( return v; } +flist_t * +flist_field_match( + const field_t *field, + fldt_t type, + void *obj, + int startoff) +{ + flist_t *fl; + int count; + const ftattr_t *fa; + + fl = flist_make(field->name); + fl->fld = field; + if (field->ftyp == type) + return fl; + count = fcount(field, obj, startoff); + if (!count) + goto out; + fa = &ftattrtab[field->ftyp]; + if (fa->subfld) { + flist_t *nfl; + + nfl = flist_find_ftyp(fa->subfld, type, obj, startoff); + if (nfl) { + fl->child = nfl; + return fl; + } + } + +out: + flist_free(fl); + return NULL; +} + /* * Given a set of fields, scan for a field of the given type. * Return an flist leading to the first found field @@ -413,33 +447,14 @@ flist_find_ftyp( void *obj, int startoff) { - flist_t *fl; const field_t *f; - int count; - const ftattr_t *fa; + flist_t *fl; for (f = fields; f->name; f++) { - fl = flist_make(f->name); - fl->fld = f; - if (f->ftyp == type) + if ((fl = flist_field_match(f, type, obj, startoff)) != NULL) return fl; - count = fcount(f, obj, startoff); - if (!count) { - flist_free(fl); - continue; - } - fa = &ftattrtab[f->ftyp]; - if (fa->subfld) { - flist_t *nfl; - - nfl = flist_find_ftyp(fa->subfld, type, obj, startoff); - if (nfl) { - fl->child = nfl; - return fl; - } - } - flist_free(fl); } + return NULL; }
Make flist_find_type() more readable by unloading field type matching to the helper. Signed-off-by: Andrey Albershteyn <aalbersh@redhat.com> --- db/flist.c | 59 ++++++++++++++++++++++++++++++++++-------------------- 1 file changed, 37 insertions(+), 22 deletions(-)