Re: [PATCH 3/8] lib: bitmap: fold nbits into region struct

From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
To: Paul Gortmaker <paul.gortmaker@windriver.com>
Cc: linux-kernel@vger.kernel.org, lizefan@huawei.com,
	mingo@kernel.org, tglx@linutronix.de, josh@joshtriplett.org,
	yury.norov@gmail.com, peterz@infradead.org, paulmck@kernel.org,
	fweisbec@gmail.com, linux@rasmusvillemoes.dk
Subject: Re: [PATCH 3/8] lib: bitmap: fold nbits into region struct
Date: Tue, 26 Jan 2021 23:18:14 +0200	[thread overview]
Message-ID: <YBCHFrO3LS+Hztx/@smile.fi.intel.com> (raw)
In-Reply-To: <YBCGqfW0hKSgo9Rl@smile.fi.intel.com>

On Tue, Jan 26, 2021 at 11:16:25PM +0200, Andy Shevchenko wrote:
> On Tue, Jan 26, 2021 at 12:11:36PM -0500, Paul Gortmaker wrote:
> > This will reduce parameter passing and enable using nbits as part
> > of future dynamic region parameter parsing.

...

> >  	struct region r;
> >  	long ret;
> >  
> > -	bitmap_zero(maskp, nmaskbits);
> > +	r.nbits = nmaskbits;

Alternatively (though I personally don't prefer it) you can clarify in the
definition block the initial values.

	struct region r = { .nbist = nmaskbits };

> > +	bitmap_zero(maskp, r.nbits);
> 
> This sounds not right from style perspective.
> You have completely uninitialized r on stack, then you assign only one value
> for immediate use here and...
> 
> >  	while (buf) {
> >  		buf = bitmap_find_region(buf);
> > @@ -655,7 +656,7 @@ int bitmap_parselist(const char *buf, unsigned long *maskp, int nmaskbits)
> >  		if (ret)
> >  			return ret;
> >  
> > -		ret = bitmap_set_region(&r, maskp, nmaskbits);
> > +		ret = bitmap_set_region(&r, maskp);
> 
> ...hiding this fact here. Which I would expect that &r may be rewritten here.
> 
> I would leave these unchanged and simple assign the value in
> bitmap_set_region().

-- 
With Best Regards,
Andy Shevchenko