Re: [PATCH] Staging: xgifb: XGI_main_26.c: Refactored the function

[Dan Carpenter]

That looks nice.  Thank you.  Sorry for the headaches.

regards,
dan carpenter

Re: [PATCH] Staging: xgifb: XGI_main_26.c: Refactored the function

[Dan Carpenter]

That looks nice.  Thank you.  Sorry for the headaches.

regards,
dan carpenter

Re: [PATCH] Staging: xgifb: XGI_main_26.c: Refactored the function

[Dan Carpenter]

On Tue, Mar 20, 2018 at 07:32:32PM +0530, Pratik Jain wrote:
> Refactored the function `XGIfb_search_refresh_rate` by removing a level
> of `if...else` block nesting. Removed unnecessary parantheses.
> 
> Signed-off-by: Pratik Jain 
> ---
>  drivers/staging/xgifb/XGI_main_26.c | 59 
> +++--
>  1 file changed, 30 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/staging/xgifb/XGI_main_26.c 
> b/drivers/staging/xgifb/XGI_main_26.c
> index 10107de0119a..aee969aab681 100644
> --- a/drivers/staging/xgifb/XGI_main_26.c
> +++ b/drivers/staging/xgifb/XGI_main_26.c
> @@ -544,41 +544,42 @@ static u8 XGIfb_search_refresh_rate(struct 
> xgifb_video_info *xgifb_info,
>   yres = XGIbios_mode[xgifb_info->mode_idx].yres;
>  
>   xgifb_info->rate_idx = 0;
> - while ((XGIfb_vrate[i].idx != 0) && (XGIfb_vrate[i].xres <= xres)) {
> - if ((XGIfb_vrate[i].xres == xres) &&
> - (XGIfb_vrate[i].yres == yres)) {
> - if (XGIfb_vrate[i].refresh == rate) {
> +
> + while (XGIfb_vrate[i].idx != 0 && XGIfb_vrate[i].xres <= xres) {
> + /* Skip values with xres or yres less than specified */
> + if ((XGIfb_vrate[i].yres != yres) ||
> + (XGIfb_vrate[i].xres < xres)) {

Both < and != are equivalent, but to me != feels more obvious.  With !=
the opposite of that is *always* == so that's obvious just by looking at
it.  But with < you have to look at the lines before so you have to
remember more stuff to understand the code.

The other thing is that != xres matches nicely with != yres.  Otherwise
I sort of assume from looking at it that != and < are different things.

regards,
dan carpenter

Re: [PATCH] Staging: xgifb: XGI_main_26.c: Refactored the function

[Dan Carpenter]

On Tue, Mar 20, 2018 at 07:32:32PM +0530, Pratik Jain wrote:
> Refactored the function `XGIfb_search_refresh_rate` by removing a level
> of `if...else` block nesting. Removed unnecessary parantheses.
> 
> Signed-off-by: Pratik Jain 
> ---
>  drivers/staging/xgifb/XGI_main_26.c | 59 
> +++--
>  1 file changed, 30 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/staging/xgifb/XGI_main_26.c 
> b/drivers/staging/xgifb/XGI_main_26.c
> index 10107de0119a..aee969aab681 100644
> --- a/drivers/staging/xgifb/XGI_main_26.c
> +++ b/drivers/staging/xgifb/XGI_main_26.c
> @@ -544,41 +544,42 @@ static u8 XGIfb_search_refresh_rate(struct 
> xgifb_video_info *xgifb_info,
>   yres = XGIbios_mode[xgifb_info->mode_idx].yres;
>  
>   xgifb_info->rate_idx = 0;
> - while ((XGIfb_vrate[i].idx != 0) && (XGIfb_vrate[i].xres <= xres)) {
> - if ((XGIfb_vrate[i].xres == xres) &&
> - (XGIfb_vrate[i].yres == yres)) {
> - if (XGIfb_vrate[i].refresh == rate) {
> +
> + while (XGIfb_vrate[i].idx != 0 && XGIfb_vrate[i].xres <= xres) {
> + /* Skip values with xres or yres less than specified */
> + if ((XGIfb_vrate[i].yres != yres) ||
> + (XGIfb_vrate[i].xres < xres)) {

Both < and != are equivalent, but to me != feels more obvious.  With !=
the opposite of that is *always* == so that's obvious just by looking at
it.  But with < you have to look at the lines before so you have to
remember more stuff to understand the code.

The other thing is that != xres matches nicely with != yres.  Otherwise
I sort of assume from looking at it that != and < are different things.

regards,
dan carpenter

Re: [PATCH] Staging: xgifb: XGI_main_26.c: Refactored the function

[Dan Carpenter]

On Tue, Mar 20, 2018 at 02:05:49PM +0530, Pratik Jain wrote:
> Refactored the function `XGIfb_search_refresh_rate` by removing a level
> of `if...else` block nesting. Removed unnecessary parantheses.
> 
> Signed-off-by: Pratik Jain 
> ---
>  drivers/staging/xgifb/XGI_main_26.c | 63 
> +++--
>  1 file changed, 33 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/staging/xgifb/XGI_main_26.c 
> b/drivers/staging/xgifb/XGI_main_26.c
> index 10107de0119a..ef9a726cd35d 100644
> --- a/drivers/staging/xgifb/XGI_main_26.c
> +++ b/drivers/staging/xgifb/XGI_main_26.c
> @@ -544,41 +544,44 @@ static u8 XGIfb_search_refresh_rate(struct 
> xgifb_video_info *xgifb_info,
>   yres = XGIbios_mode[xgifb_info->mode_idx].yres;
>  
>   xgifb_info->rate_idx = 0;
> - while ((XGIfb_vrate[i].idx != 0) && (XGIfb_vrate[i].xres <= xres)) {
> - if ((XGIfb_vrate[i].xres == xres) &&
> - (XGIfb_vrate[i].yres == yres)) {
> - if (XGIfb_vrate[i].refresh == rate) {
> + 

There is a stray tab here.  You didn't run checkpatch.pl.

> + // Skip values with less xres

Linus likes this comment style, but I would prefer normal comments,
please.

> + while (XGIfb_vrate[i].idx != 0 && XGIfb_vrate[i].xres < xres)
> + ++i;
> +

I have reviewed the code, and I still find the single loop more
readable.

> + while (XGIfb_vrate[i].idx != 0 && XGIfb_vrate[i].xres <= xres) {
> + if (XGIfb_vrate[i].yres != yres) {
> + ++i;
> + continue;
> + }

I would like a change that did:

if ((XGIfb_vrate[i].xres != xres) ||
(XGIfb_vrate[i].yres != yres)) {
i++;
continue;
}

so we could pull everything in one tab.

> + if (XGIfb_vrate[i].refresh == rate) {
> + xgifb_info->rate_idx = XGIfb_vrate[i].idx;
> + break;
> + } else if (XGIfb_vrate[i].refresh > rate) {
> + if (XGIfb_vrate[i].refresh - rate <= 3) {
> + pr_debug("Adjusting rate from %d up to %d\n",
> + rate, XGIfb_vrate[i].refresh);
>   xgifb_info->rate_idx = XGIfb_vrate[i].idx;
> - break;
> - } else if (XGIfb_vrate[i].refresh > rate) {
> - if ((XGIfb_vrate[i].refresh - rate) <= 3) {
> - pr_debug("Adjusting rate from %d up to 
> %d\n",
> -  rate, XGIfb_vrate[i].refresh);
> - xgifb_info->rate_idx =
> - XGIfb_vrate[i].idx;
> - xgifb_info->refresh_rate =
> - XGIfb_vrate[i].refresh;
> - } else if (((rate - XGIfb_vrate[i - 1].refresh)
> - <= 2) && (XGIfb_vrate[i].idx
> - != 1)) {
> - pr_debug("Adjusting rate from %d down 
> to %d\n",
> -  rate,
> -  XGIfb_vrate[i - 1].refresh);
> - xgifb_info->rate_idx =
> - XGIfb_vrate[i - 1].idx;
> - xgifb_info->refresh_rate =
> - XGIfb_vrate[i - 1].refresh;
> - }
> - break;
> - } else if ((rate - XGIfb_vrate[i].refresh) <= 2) {
> + xgifb_info->refresh_rate =
> + XGIfb_vrate[i].refresh;
> + } else if ((rate - XGIfb_vrate[i - 1].refresh <= 2)
> + && (XGIfb_vrate[i].idx != 1)) {
^^^
This bug is there in the original code, and not something that you
introduced but the second part of the if condition is to ensure that
we didn't do an array underflow in the first part of the if statement.

These days that can trigger a kasan warning, I believe.

The conditions should be swapped around to avoid the read before the
start of the array altogether.  And, in fact, it should be written like
this to make it easier for static analysis tools:

} else if (i != 0 &&
   rate - XGIfb_vrate[i - 1].refresh <= 2) {

regards,
dan carpenter

Re: [PATCH] Staging: xgifb: XGI_main_26.c: Refactored the function

[Dan Carpenter]

On Tue, Mar 20, 2018 at 02:05:49PM +0530, Pratik Jain wrote:
> Refactored the function `XGIfb_search_refresh_rate` by removing a level
> of `if...else` block nesting. Removed unnecessary parantheses.
> 
> Signed-off-by: Pratik Jain 
> ---
>  drivers/staging/xgifb/XGI_main_26.c | 63 
> +++--
>  1 file changed, 33 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/staging/xgifb/XGI_main_26.c 
> b/drivers/staging/xgifb/XGI_main_26.c
> index 10107de0119a..ef9a726cd35d 100644
> --- a/drivers/staging/xgifb/XGI_main_26.c
> +++ b/drivers/staging/xgifb/XGI_main_26.c
> @@ -544,41 +544,44 @@ static u8 XGIfb_search_refresh_rate(struct 
> xgifb_video_info *xgifb_info,
>   yres = XGIbios_mode[xgifb_info->mode_idx].yres;
>  
>   xgifb_info->rate_idx = 0;
> - while ((XGIfb_vrate[i].idx != 0) && (XGIfb_vrate[i].xres <= xres)) {
> - if ((XGIfb_vrate[i].xres == xres) &&
> - (XGIfb_vrate[i].yres == yres)) {
> - if (XGIfb_vrate[i].refresh == rate) {
> + 

There is a stray tab here.  You didn't run checkpatch.pl.

> + // Skip values with less xres

Linus likes this comment style, but I would prefer normal comments,
please.

> + while (XGIfb_vrate[i].idx != 0 && XGIfb_vrate[i].xres < xres)
> + ++i;
> +

I have reviewed the code, and I still find the single loop more
readable.

> + while (XGIfb_vrate[i].idx != 0 && XGIfb_vrate[i].xres <= xres) {
> + if (XGIfb_vrate[i].yres != yres) {
> + ++i;
> + continue;
> + }

I would like a change that did:

if ((XGIfb_vrate[i].xres != xres) ||
(XGIfb_vrate[i].yres != yres)) {
i++;
continue;
}

so we could pull everything in one tab.

> + if (XGIfb_vrate[i].refresh == rate) {
> + xgifb_info->rate_idx = XGIfb_vrate[i].idx;
> + break;
> + } else if (XGIfb_vrate[i].refresh > rate) {
> + if (XGIfb_vrate[i].refresh - rate <= 3) {
> + pr_debug("Adjusting rate from %d up to %d\n",
> + rate, XGIfb_vrate[i].refresh);
>   xgifb_info->rate_idx = XGIfb_vrate[i].idx;
> - break;
> - } else if (XGIfb_vrate[i].refresh > rate) {
> - if ((XGIfb_vrate[i].refresh - rate) <= 3) {
> - pr_debug("Adjusting rate from %d up to 
> %d\n",
> -  rate, XGIfb_vrate[i].refresh);
> - xgifb_info->rate_idx =
> - XGIfb_vrate[i].idx;
> - xgifb_info->refresh_rate =
> - XGIfb_vrate[i].refresh;
> - } else if (((rate - XGIfb_vrate[i - 1].refresh)
> - <= 2) && (XGIfb_vrate[i].idx
> - != 1)) {
> - pr_debug("Adjusting rate from %d down 
> to %d\n",
> -  rate,
> -  XGIfb_vrate[i - 1].refresh);
> - xgifb_info->rate_idx =
> - XGIfb_vrate[i - 1].idx;
> - xgifb_info->refresh_rate =
> - XGIfb_vrate[i - 1].refresh;
> - }
> - break;
> - } else if ((rate - XGIfb_vrate[i].refresh) <= 2) {
> + xgifb_info->refresh_rate =
> + XGIfb_vrate[i].refresh;
> + } else if ((rate - XGIfb_vrate[i - 1].refresh <= 2)
> + && (XGIfb_vrate[i].idx != 1)) {
^^^
This bug is there in the original code, and not something that you
introduced but the second part of the if condition is to ensure that
we didn't do an array underflow in the first part of the if statement.

These days that can trigger a kasan warning, I believe.

The conditions should be swapped around to avoid the read before the
start of the array altogether.  And, in fact, it should be written like
this to make it easier for static analysis tools:

} else if (i != 0 &&
   rate - XGIfb_vrate[i - 1].refresh <= 2) {

regards,
dan carpenter

Re: [PATCH] Staging: xgifb: XGI_main_26.c: Refactored the function

[Pratik Jain]

You got a valid point about `++i` and `i++`. But I still feel
that it is less complicated than previous one. I am explicitly
saying(in first loop) that we are skipping some values to get
to a specific index. Apart from that, we can avoid unncessary
wrapping and indentation. Logic becomes much more explicit if
broken down to two loops.

Re: [PATCH] Staging: xgifb: XGI_main_26.c: Refactored the function

[Pratik Jain]

You got a valid point about `++i` and `i++`. But I still feel
that it is less complicated than previous one. I am explicitly
saying(in first loop) that we are skipping some values to get
to a specific index. Apart from that, we can avoid unncessary
wrapping and indentation. Logic becomes much more explicit if
broken down to two loops.

Re: [PATCH] Staging: xgifb: XGI_main_26.c: Refactored the function

[Dan Carpenter]

I'm trying to review this, but I feel like this makes it slightly more
complicated for no reason.  Why break it up into two loops?

> - i++;
> + ++i;

These are equivalent, so you should default to accepting the original
author's style.  Otherwise the next person to touch this code will just
change it back and we get into a cycle of pointless changes.

regards,
dan carpenter

Re: [PATCH] Staging: xgifb: XGI_main_26.c: Refactored the function

[Dan Carpenter]

I'm trying to review this, but I feel like this makes it slightly more
complicated for no reason.  Why break it up into two loops?

> - i++;
> + ++i;

These are equivalent, so you should default to accepting the original
author's style.  Otherwise the next person to touch this code will just
change it back and we get into a cycle of pointless changes.

regards,
dan carpenter