Re: [PATCH 5/8] lib: bitmap_getnum: separate arg into region and field

[Paul Gortmaker <paul.gortmaker@windriver.com>]

[Re: [PATCH 5/8] lib: bitmap_getnum: separate arg into region and field] On 26/01/2021 (Tue 18:58) Yury Norov wrote:

> On Tue, Jan 26, 2021 at 1:22 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> >
> > On Tue, Jan 26, 2021 at 12:11:38PM -0500, Paul Gortmaker wrote:
> > > The bitmap_getnum is only used on a region's start/end/off/group_len
> > > field.  Trivially decouple the region from the field so that the region
> > > pointer is available for a pending change.
> >
> > Honestly, I don't like this macro trick. It's bad in couple of ways:
> >  - it hides what actually is done with the fields of r structure
> >    (after you get that they are fields!)
> >  - it breaks possibility to compile time (type) checks
> >
> > I will listen what others say, but I'm in favour not to proceed like this.
> 
> Agree. Would be better to drop the patch. Paul, what kind of pending
> change do you mean here? All the following patches are not related to
> parsing machinery.

It was directly related, because...

> 
> > > Cc: Yury Norov <yury.norov@gmail.com>
> > > Cc: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> > > Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > > Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
> > > ---
> > >  lib/bitmap.c | 9 +++++----
> > >  1 file changed, 5 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/lib/bitmap.c b/lib/bitmap.c
> > > index 833f152a2c43..f65be2f148fd 100644
> > > --- a/lib/bitmap.c
> > > +++ b/lib/bitmap.c
> > > @@ -533,6 +533,7 @@ static const char *bitmap_getnum(const char *str, unsigned int *num)
> > >       *num = n;
> > >       return str + len;
> > >  }
> > > +#define bitmap_getrnum(s, r, pos) bitmap_getnum(s, &(r->pos))

...this one line above opened the door to then do [in 6/8]:

   -#define bitmap_getrnum(s, r, pos) bitmap_getnum(s, &(r->pos))
   +#define bitmap_getrnum(s, r, pos) __bitmap_getnum(s, r->nbits, &(r->pos))

which gets nbits down into bitmap_getnum so we can handle N in there as
the placement you'd specifically requested for treating N as just a number.

In any case, I've decided against putting nbits into the region struct
and have got the nbits value down into getnum() another way for v4,
without using this commit or similar macros.

Paul.
--

> > >
> > >  static inline bool end_of_str(char c)
> > >  {
> > > @@ -571,7 +572,7 @@ static const char *bitmap_find_region_reverse(const char *start, const char *end
> > >
> > >  static const char *bitmap_parse_region(const char *str, struct region *r)
> > >  {
> > > -     str = bitmap_getnum(str, &r->start);
> > > +     str = bitmap_getrnum(str, r, start);
> > >       if (IS_ERR(str))
> > >               return str;
> > >
> > > @@ -581,7 +582,7 @@ static const char *bitmap_parse_region(const char *str, struct region *r)
> > >       if (*str != '-')
> > >               return ERR_PTR(-EINVAL);
> > >
> > > -     str = bitmap_getnum(str + 1, &r->end);
> > > +     str = bitmap_getrnum(str + 1, r, end);
> > >       if (IS_ERR(str))
> > >               return str;
> > >
> > > @@ -591,14 +592,14 @@ static const char *bitmap_parse_region(const char *str, struct region *r)
> > >       if (*str != ':')
> > >               return ERR_PTR(-EINVAL);
> > >
> > > -     str = bitmap_getnum(str + 1, &r->off);
> > > +     str = bitmap_getrnum(str + 1, r, off);
> > >       if (IS_ERR(str))
> > >               return str;
> > >
> > >       if (*str != '/')
> > >               return ERR_PTR(-EINVAL);
> > >
> > > -     return bitmap_getnum(str + 1, &r->group_len);
> > > +     return bitmap_getrnum(str + 1, r, group_len);
> > >
> > >  no_end:
> > >       r->end = r->start;
> > > --
> > > 2.17.1
> > >
> >
> > --
> > With Best Regards,
> > Andy Shevchenko
> >
> >