Re: [Outreachy kernel] [PATCH v2 3/4] staging: fbtft: simplify array index computation

[Stefano Brivio <sbrivio@redhat.com>]

Hmm, this isn't exactly simplifying if one looks at the diffstat:

On Fri, 13 Mar 2020 19:14:02 +0530
Deepak R Varma <mh12gx2825@gmail.com> wrote:

> An array index is being computed by mathematical calculation on the
> Lvalue side of the expression. This also further results in the staement
> exceeding 80 character statement length.
> 
> A local variable can store the value of the array index computation. The
> variable can then be used as array index. This improves readability of
> the code and also address 80 character warning raised by checkpatch.
> 
> Signed-off-by: Deepak R Varma <mh12gx2825@gmail.com>
> ---
>  drivers/staging/fbtft/fbtft-sysfs.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

More on that below:

> 
> diff --git a/drivers/staging/fbtft/fbtft-sysfs.c b/drivers/staging/fbtft/fbtft-sysfs.c
> index 2a5c630dab87..4d505a13f7ab 100644
> --- a/drivers/staging/fbtft/fbtft-sysfs.c
> +++ b/drivers/staging/fbtft/fbtft-sysfs.c
> @@ -25,6 +25,7 @@ int fbtft_gamma_parse_str(struct fbtft_par *par, u32 *curves,
>  	unsigned long val = 0;
>  	int ret = 0;
>  	int curve_counter, value_counter;
> +	int counter = 0;

...there's no need to initialise this.

>  
>  	fbtft_par_dbg(DEBUG_SYSFS, par, "%s() str=\n", __func__);
>  
> @@ -68,7 +69,10 @@ int fbtft_gamma_parse_str(struct fbtft_par *par, u32 *curves,
>  			ret = get_next_ulong(&curve_p, &val, " ", 16);
>  			if (ret)
>  				goto out;
> -			curves[curve_counter * par->gamma.num_values + value_counter] = val;
> +
> +			counter = curve_counter * par->gamma.num_values +
> +					value_counter;

value_counter is summed to the whole thing, so it should be aligned
under curve_counter.

> +			curves[counter] = val;

And here, I think, the actual problem is variable naming itself. See
section "4) Naming" of Documentation/process/coding-style.rst -- this
looks a bit like Java. The "_counter" suffix doesn't add much here, I
guess those could just be "curve" and "value". I haven't checked for
ambiguities (could you have a look?), but if there's any, even "_count"
would make this a bit more tolerable.

-- 
Stefano