* [PATCH v6] staging: xgifb: correct the multiple line dereference
@ 2017-02-28 5:05 Arushi Singhal
2017-02-28 5:51 ` Greg Kroah-Hartman
0 siblings, 1 reply; 8+ messages in thread
From: Arushi Singhal @ 2017-02-28 5:05 UTC (permalink / raw)
To: arnaud.patard; +Cc: Greg Kroah-Hartman, devel, linux-kernel, outreachy-kernel
Error reported by checkpatch.pl as "avoid multiple line dereference".
Addition of new variables to make the code more readable and also to
correct about mentioned error as by itroducing new variables line is
not exceeding 80 characters.
Signed-off-by: Arushi Singhal <arushisinghal19971997@gmail.com>
---
changes in v6
- changes done such that no other errors can generate.
- Improve the coding style.
- Introduced new variables.
- type of the variable is changed.
drivers/staging/xgifb/XGI_main_26.c | 29 ++++++-----------------------
drivers/staging/xgifb/vb_setmode.c | 17 +++++++++++------
2 files changed, 17 insertions(+), 29 deletions(-)
diff --git a/drivers/staging/xgifb/XGI_main_26.c b/drivers/staging/xgifb/XGI_main_26.c
index 69ed137337ce..9870ea3b76b4 100644
--- a/drivers/staging/xgifb/XGI_main_26.c
+++ b/drivers/staging/xgifb/XGI_main_26.c
@@ -878,30 +878,13 @@ static void XGIfb_post_setmode(struct xgifb_video_info *xgifb_info)
}
if ((filter >= 0) && (filter <= 7)) {
+ const u8 *f = XGI_TV_filter[filter_tb].filter[filter];
pr_debug("FilterTable[%d]-%d: %*ph\n",
- filter_tb, filter,
- 4, XGI_TV_filter[filter_tb].
- filter[filter]);
- xgifb_reg_set(
- XGIPART2,
- 0x35,
- (XGI_TV_filter[filter_tb].
- filter[filter][0]));
- xgifb_reg_set(
- XGIPART2,
- 0x36,
- (XGI_TV_filter[filter_tb].
- filter[filter][1]));
- xgifb_reg_set(
- XGIPART2,
- 0x37,
- (XGI_TV_filter[filter_tb].
- filter[filter][2]));
- xgifb_reg_set(
- XGIPART2,
- 0x38,
- (XGI_TV_filter[filter_tb].
- filter[filter][3]));
+ filter_tb, filter, 4, f);
+ xgifb_reg_set(XGIPART2, 0x35, f[0]);
+ xgifb_reg_set(XGIPART2, 0x36, f[1]);
+ xgifb_reg_set(XGIPART2, 0x37, f[2]);
+ xgifb_reg_set(XGIPART2, 0x38, f[3]);
}
}
}
diff --git a/drivers/staging/xgifb/vb_setmode.c b/drivers/staging/xgifb/vb_setmode.c
index 7c7c8c8f1df3..249a32804c06 100644
--- a/drivers/staging/xgifb/vb_setmode.c
+++ b/drivers/staging/xgifb/vb_setmode.c
@@ -221,8 +221,11 @@ static unsigned char XGI_AjustCRT2Rate(unsigned short ModeIdIndex,
for (; XGI330_RefIndex[RefreshRateTableIndex + (*i)].ModeID ==
tempbx; (*i)--) {
- infoflag = XGI330_RefIndex[RefreshRateTableIndex + (*i)].
- Ext_InfoFlag;
+ unsigned short j;
+
+ j = XGI330_RefIndex[RefreshRateTableIndex + (*i)].Ext_InfoFlag;
+ infoflag = j;
+
if (infoflag & tempax)
return 1;
@@ -231,8 +234,11 @@ static unsigned char XGI_AjustCRT2Rate(unsigned short ModeIdIndex,
}
for ((*i) = 0;; (*i)++) {
- infoflag = XGI330_RefIndex[RefreshRateTableIndex + (*i)].
- Ext_InfoFlag;
+ unsigned short m;
+
+ m = XGI330_RefIndex[RefreshRateTableIndex + (*i)].Ext_InfoFlag;
+ infoflag = m;
+
if (XGI330_RefIndex[RefreshRateTableIndex + (*i)].ModeID
!= tempbx) {
return 0;
@@ -5092,8 +5098,7 @@ unsigned short XGI_GetRatePtrCRT2(struct xgi_hw_device_info *pXGIHWDE,
i = 0;
do {
- if (XGI330_RefIndex[RefreshRateTableIndex + i].
- ModeID != ModeNo)
+ if (XGI330_RefIndex[RefreshRateTableIndex + i].ModeID != ModeNo)
break;
temp = XGI330_RefIndex[RefreshRateTableIndex + i].Ext_InfoFlag;
temp &= ModeTypeMask;
--
2.11.0
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v6] staging: xgifb: correct the multiple line dereference
2017-02-28 5:05 [PATCH v6] staging: xgifb: correct the multiple line dereference Arushi Singhal
@ 2017-02-28 5:51 ` Greg Kroah-Hartman
2017-02-28 6:19 ` Joe Perches
2017-02-28 8:29 ` Arushi Singhal
0 siblings, 2 replies; 8+ messages in thread
From: Greg Kroah-Hartman @ 2017-02-28 5:51 UTC (permalink / raw)
To: Arushi Singhal; +Cc: arnaud.patard, devel, linux-kernel, outreachy-kernel
On Tue, Feb 28, 2017 at 10:35:30AM +0530, Arushi Singhal wrote:
> Error reported by checkpatch.pl as "avoid multiple line dereference".
> Addition of new variables to make the code more readable and also to
> correct about mentioned error as by itroducing new variables line is
> not exceeding 80 characters.
>
> Signed-off-by: Arushi Singhal <arushisinghal19971997@gmail.com>
> ---
> changes in v6
> - changes done such that no other errors can generate.
> - Improve the coding style.
> - Introduced new variables.
> - type of the variable is changed.
>
> drivers/staging/xgifb/XGI_main_26.c | 29 ++++++-----------------------
> drivers/staging/xgifb/vb_setmode.c | 17 +++++++++++------
> 2 files changed, 17 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/staging/xgifb/XGI_main_26.c b/drivers/staging/xgifb/XGI_main_26.c
> index 69ed137337ce..9870ea3b76b4 100644
> --- a/drivers/staging/xgifb/XGI_main_26.c
> +++ b/drivers/staging/xgifb/XGI_main_26.c
> @@ -878,30 +878,13 @@ static void XGIfb_post_setmode(struct xgifb_video_info *xgifb_info)
> }
>
> if ((filter >= 0) && (filter <= 7)) {
> + const u8 *f = XGI_TV_filter[filter_tb].filter[filter];
> pr_debug("FilterTable[%d]-%d: %*ph\n",
> - filter_tb, filter,
> - 4, XGI_TV_filter[filter_tb].
> - filter[filter]);
> - xgifb_reg_set(
> - XGIPART2,
> - 0x35,
> - (XGI_TV_filter[filter_tb].
> - filter[filter][0]));
> - xgifb_reg_set(
> - XGIPART2,
> - 0x36,
> - (XGI_TV_filter[filter_tb].
> - filter[filter][1]));
> - xgifb_reg_set(
> - XGIPART2,
> - 0x37,
> - (XGI_TV_filter[filter_tb].
> - filter[filter][2]));
> - xgifb_reg_set(
> - XGIPART2,
> - 0x38,
> - (XGI_TV_filter[filter_tb].
> - filter[filter][3]));
> + filter_tb, filter, 4, f);
> + xgifb_reg_set(XGIPART2, 0x35, f[0]);
> + xgifb_reg_set(XGIPART2, 0x36, f[1]);
> + xgifb_reg_set(XGIPART2, 0x37, f[2]);
> + xgifb_reg_set(XGIPART2, 0x38, f[3]);
> }
> }
> }
> diff --git a/drivers/staging/xgifb/vb_setmode.c b/drivers/staging/xgifb/vb_setmode.c
> index 7c7c8c8f1df3..249a32804c06 100644
> --- a/drivers/staging/xgifb/vb_setmode.c
> +++ b/drivers/staging/xgifb/vb_setmode.c
> @@ -221,8 +221,11 @@ static unsigned char XGI_AjustCRT2Rate(unsigned short ModeIdIndex,
>
> for (; XGI330_RefIndex[RefreshRateTableIndex + (*i)].ModeID ==
> tempbx; (*i)--) {
> - infoflag = XGI330_RefIndex[RefreshRateTableIndex + (*i)].
> - Ext_InfoFlag;
> + unsigned short j;
> +
> + j = XGI330_RefIndex[RefreshRateTableIndex + (*i)].Ext_InfoFlag;
> + infoflag = j;
> +
> if (infoflag & tempax)
> return 1;
Why are you using a temporary variable 'j' here? It's not needed at
all, and just is confusing to read the code now, don't you agree?
> @@ -231,8 +234,11 @@ static unsigned char XGI_AjustCRT2Rate(unsigned short ModeIdIndex,
> }
>
> for ((*i) = 0;; (*i)++) {
> - infoflag = XGI330_RefIndex[RefreshRateTableIndex + (*i)].
> - Ext_InfoFlag;
> + unsigned short m;
> +
> + m = XGI330_RefIndex[RefreshRateTableIndex + (*i)].Ext_InfoFlag;
> + infoflag = m;
> +
> if (XGI330_RefIndex[RefreshRateTableIndex + (*i)].ModeID
> != tempbx) {
> return 0;
Same here, why add a new variable that isn't used more than once? You
are trying to work around something that doesn't make sense to work
around.
Remember, coding style cleanups are to be done to make the code easier
to understand and follow. Not to blindly follow a perl script that
can not think. Sometimes it is not right...
thanks,
greg k-h
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v6] staging: xgifb: correct the multiple line dereference
2017-02-28 5:51 ` Greg Kroah-Hartman
@ 2017-02-28 6:19 ` Joe Perches
2017-02-28 8:29 ` Arushi Singhal
1 sibling, 0 replies; 8+ messages in thread
From: Joe Perches @ 2017-02-28 6:19 UTC (permalink / raw)
To: Greg Kroah-Hartman, Arushi Singhal
Cc: arnaud.patard, devel, linux-kernel, outreachy-kernel
On Tue, 2017-02-28 at 06:51 +0100, Greg Kroah-Hartman wrote:
> On Tue, Feb 28, 2017 at 10:35:30AM +0530, Arushi Singhal wrote:
> > Error reported by checkpatch.pl as "avoid multiple line dereference".
> > Addition of new variables to make the code more readable and also to
> > correct about mentioned error as by itroducing new variables line is
> > not exceeding 80 characters.
[]
> Remember, coding style cleanups are to be done to make the code easier
> to understand and follow. Not to blindly follow a perl script that
> can not think. Sometimes it is not right...
Yeah, what Greg said.
Also very long identifier names like RefreshRateTableIndex
and simple dereferences like
XGI330_RefIndex[RefreshRateTableIndex + (*i)].Ext_InfoFlag
make using 80 columns silly.
Just ignore 80 column limits when there are 20+ character
identifiers. Ideally shorten the identifiers to something
less verbose and/or use temporaries where appropriate like
I showed in my reply to your suggested V5 patch.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v6] staging: xgifb: correct the multiple line dereference
2017-02-28 5:51 ` Greg Kroah-Hartman
2017-02-28 6:19 ` Joe Perches
@ 2017-02-28 8:29 ` Arushi Singhal
2017-02-28 8:44 ` [Outreachy kernel] " Julia Lawall
2017-02-28 9:37 ` Greg Kroah-Hartman
1 sibling, 2 replies; 8+ messages in thread
From: Arushi Singhal @ 2017-02-28 8:29 UTC (permalink / raw)
To: Greg Kroah-Hartman; +Cc: arnaud.patard, devel, linux-kernel, outreachy-kernel
[-- Attachment #1: Type: text/plain, Size: 5369 bytes --]
On Tue, Feb 28, 2017 at 11:21 AM, Greg Kroah-Hartman <
gregkh@linuxfoundation.org> wrote:
> On Tue, Feb 28, 2017 at 10:35:30AM +0530, Arushi Singhal wrote:
> > Error reported by checkpatch.pl as "avoid multiple line dereference".
> > Addition of new variables to make the code more readable and also to
> > correct about mentioned error as by itroducing new variables line is
> > not exceeding 80 characters.
> >
> > Signed-off-by: Arushi Singhal <arushisinghal19971997@gmail.com>
> > ---
> > changes in v6
> > - changes done such that no other errors can generate.
> > - Improve the coding style.
> > - Introduced new variables.
> > - type of the variable is changed.
> >
> > drivers/staging/xgifb/XGI_main_26.c | 29 ++++++-----------------------
> > drivers/staging/xgifb/vb_setmode.c | 17 +++++++++++------
> > 2 files changed, 17 insertions(+), 29 deletions(-)
> >
> > diff --git a/drivers/staging/xgifb/XGI_main_26.c
> b/drivers/staging/xgifb/XGI_main_26.c
> > index 69ed137337ce..9870ea3b76b4 100644
> > --- a/drivers/staging/xgifb/XGI_main_26.c
> > +++ b/drivers/staging/xgifb/XGI_main_26.c
> > @@ -878,30 +878,13 @@ static void XGIfb_post_setmode(struct
> xgifb_video_info *xgifb_info)
> > }
> >
> > if ((filter >= 0) && (filter <= 7)) {
> > + const u8 *f = XGI_TV_filter[filter_tb].
> filter[filter];
> > pr_debug("FilterTable[%d]-%d: %*ph\n",
> > - filter_tb, filter,
> > - 4, XGI_TV_filter[filter_tb].
> > - filter[filter]);
> > - xgifb_reg_set(
> > - XGIPART2,
> > - 0x35,
> > - (XGI_TV_filter[filter_tb].
> > - filter[filter][0]));
> > - xgifb_reg_set(
> > - XGIPART2,
> > - 0x36,
> > - (XGI_TV_filter[filter_tb].
> > - filter[filter][1]));
> > - xgifb_reg_set(
> > - XGIPART2,
> > - 0x37,
> > - (XGI_TV_filter[filter_tb].
> > - filter[filter][2]));
> > - xgifb_reg_set(
> > - XGIPART2,
> > - 0x38,
> > - (XGI_TV_filter[filter_tb].
> > - filter[filter][3]));
> > + filter_tb, filter, 4, f);
> > + xgifb_reg_set(XGIPART2, 0x35, f[0]);
> > + xgifb_reg_set(XGIPART2, 0x36, f[1]);
> > + xgifb_reg_set(XGIPART2, 0x37, f[2]);
> > + xgifb_reg_set(XGIPART2, 0x38, f[3]);
> > }
> > }
> > }
> > diff --git a/drivers/staging/xgifb/vb_setmode.c
> b/drivers/staging/xgifb/vb_setmode.c
> > index 7c7c8c8f1df3..249a32804c06 100644
> > --- a/drivers/staging/xgifb/vb_setmode.c
> > +++ b/drivers/staging/xgifb/vb_setmode.c
> > @@ -221,8 +221,11 @@ static unsigned char XGI_AjustCRT2Rate(unsigned
> short ModeIdIndex,
> >
> > for (; XGI330_RefIndex[RefreshRateTableIndex + (*i)].ModeID ==
> > tempbx; (*i)--) {
> > - infoflag = XGI330_RefIndex[RefreshRateTableIndex + (*i)].
> > - Ext_InfoFlag;
> > + unsigned short j;
> > +
> > + j = XGI330_RefIndex[RefreshRateTableIndex +
> (*i)].Ext_InfoFlag;
> > + infoflag = j;
> > +
> > if (infoflag & tempax)
> > return 1;
>
>
> Why are you using a temporary variable 'j' here? It's not needed at
> all, and just is confusing to read the code now, don't you agree?
>
>
I am using temporary variable of small size(character) so that when
I fixed the multiple line dereference then the line number of characters in
a line will
not increase by 80.
> > @@ -231,8 +234,11 @@ static unsigned char XGI_AjustCRT2Rate(unsigned
> short ModeIdIndex,
> > }0
> >
> > for ((*i) = 0;; (*i)++) {
> > - infoflag = XGI330_RefIndex[RefreshRateTableIndex + (*i)].
> > - Ext_InfoFlag;
> > + unsigned short m;
> > +
> > + m = XGI330_RefIndex[RefreshRateTableIndex +
> (*i)].Ext_InfoFlag;
> > + infoflag = m;
> > +
> > if (XGI330_RefIndex[RefreshRateTableIndex + (*i)].ModeID
> > != tempbx) {
> > return 0;
>
> Same here, why add a new variable that isn't used more than once? You
> are trying to work around something that doesn't make sense to work
> around.
>
> Same reason as above.
Thanks
Arushi
> Remember, coding style cleanups are to be done to make the code easier
> to understand and follow. Not to blindly follow a perl script that
> can not think. Sometimes it is not right...
>
> thanks,
>
> greg k-h
>
[-- Attachment #2: Type: text/html, Size: 7731 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Outreachy kernel] Re: [PATCH v6] staging: xgifb: correct the multiple line dereference
2017-02-28 8:29 ` Arushi Singhal
@ 2017-02-28 8:44 ` Julia Lawall
2017-02-28 9:37 ` Greg Kroah-Hartman
1 sibling, 0 replies; 8+ messages in thread
From: Julia Lawall @ 2017-02-28 8:44 UTC (permalink / raw)
To: Arushi Singhal
Cc: Greg Kroah-Hartman, arnaud.patard, devel, linux-kernel,
outreachy-kernel
[-- Attachment #1: Type: text/plain, Size: 7854 bytes --]
On Tue, 28 Feb 2017, Arushi Singhal wrote:
>
>
> On Tue, Feb 28, 2017 at 11:21 AM, Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
> On Tue, Feb 28, 2017 at 10:35:30AM +0530, Arushi Singhal wrote:
> > Error reported by checkpatch.pl as "avoid multiple line dereference".
> > Addition of new variables to make the code more readable and also to
> > correct about mentioned error as by itroducing new variables line is
> > not exceeding 80 characters.
> >
> > Signed-off-by: Arushi Singhal <arushisinghal19971997@gmail.com>
> > ---
> > changes in v6
> > - changes done such that no other errors can generate.
> > - Improve the coding style.
> > - Introduced new variables.
> > - type of the variable is changed.
> >
> > drivers/staging/xgifb/XGI_main_26.c | 29 ++++++-----------------------
> > drivers/staging/xgifb/vb_setmode.c | 17 +++++++++++------
> > 2 files changed, 17 insertions(+), 29 deletions(-)
> >
> > diff --git a/drivers/staging/xgifb/XGI_main_26.c b/drivers/staging/xgifb/XGI_main_26.c
> > index 69ed137337ce..9870ea3b76b4 100644
> > --- a/drivers/staging/xgifb/XGI_main_26.c
> > +++ b/drivers/staging/xgifb/XGI_main_26.c
> > @@ -878,30 +878,13 @@ static void XGIfb_post_setmode(struct xgifb_video_info *xgifb_info)
> > }
> >
> > if ((filter >= 0) && (filter <= 7)) {
> > + const u8 *f = XGI_TV_filter[filter_tb].filter[filter];
> > pr_debug("FilterTable[%d]-%d: %*ph\n",
> > - filter_tb, filter,
> > - 4, XGI_TV_filter[filter_tb].
> > - filter[filter]);
> > - xgifb_reg_set(
> > - XGIPART2,
> > - 0x35,
> > - (XGI_TV_filter[filter_tb].
> > - filter[filter][0]));
> > - xgifb_reg_set(
> > - XGIPART2,
> > - 0x36,
> > - (XGI_TV_filter[filter_tb].
> > - filter[filter][1]));
> > - xgifb_reg_set(
> > - XGIPART2,
> > - 0x37,
> > - (XGI_TV_filter[filter_tb].
> > - filter[filter][2]));
> > - xgifb_reg_set(
> > - XGIPART2,
> > - 0x38,
> > - (XGI_TV_filter[filter_tb].
> > - filter[filter][3]));
> > + filter_tb, filter, 4, f);
> > + xgifb_reg_set(XGIPART2, 0x35, f[0]);
> > + xgifb_reg_set(XGIPART2, 0x36, f[1]);
> > + xgifb_reg_set(XGIPART2, 0x37, f[2]);
> > + xgifb_reg_set(XGIPART2, 0x38, f[3]);
> > }
> > }
> > }
> > diff --git a/drivers/staging/xgifb/vb_setmode.c b/drivers/staging/xgifb/vb_setmode.c
> > index 7c7c8c8f1df3..249a32804c06 100644
> > --- a/drivers/staging/xgifb/vb_setmode.c
> > +++ b/drivers/staging/xgifb/vb_setmode.c
> > @@ -221,8 +221,11 @@ static unsigned char XGI_AjustCRT2Rate(unsigned short ModeIdIndex,
> >
> > for (; XGI330_RefIndex[RefreshRateTableIndex + (*i)].ModeID ==
> > tempbx; (*i)--) {
> > - infoflag = XGI330_RefIndex[RefreshRateTableIndex + (*i)].
> > - Ext_InfoFlag;
> > + unsigned short j;
> > +
> > + j = XGI330_RefIndex[RefreshRateTableIndex + (*i)].Ext_InfoFlag;
> > + infoflag = j;
> > +
> > if (infoflag & tempax)
> > return 1;
>
>
> Why are you using a temporary variable 'j' here? It's not needed at
> all, and just is confusing to read the code now, don't you agree?
>
>
> I am using temporary variable of small size(character) so that when
> I fixed the multiple line dereference then the line number of characters in a line will
> not increase by 80.
I agree with Greg that this is not a readable solution. Putting the whole
thing on one line would be better, even if it goes over 80 characters.
Having the field on a line by itself is much worse, because then it looks
like a variable name - one doesn't easily see the connection to the
structure. Hopefully someone will come up with a shorter name for
RefreshRateTableIndex and then the problem will be completely solved.
Maybe ref_table_index? Although to me ref looks like reference, not
refresh...
julia
>
> > @@ -231,8 +234,11 @@ static unsigned char XGI_AjustCRT2Rate(unsigned short ModeIdIndex,
> > }0
> >
> > for ((*i) = 0;; (*i)++) {
> > - infoflag = XGI330_RefIndex[RefreshRateTableIndex + (*i)].
> > - Ext_InfoFlag;
> > + unsigned short m;
> > +
> > + m = XGI330_RefIndex[RefreshRateTableIndex + (*i)].Ext_InfoFlag;
> > + infoflag = m;
> > +
> > if (XGI330_RefIndex[RefreshRateTableIndex + (*i)].ModeID
> > != tempbx) {
> > return 0;
>
> Same here, why add a new variable that isn't used more than once? You
> are trying to work around something that doesn't make sense to work
> around.
>
> Same reason as above.
> Thanks
> Arushi
>
> Remember, coding style cleanups are to be done to make the code easier
> to understand and follow. Not to blindly follow a perl script that
> can not think. Sometimes it is not right...
>
> thanks,
>
> greg k-h
>
>
>
>
>
> --
> You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com.
> To post to this group, send email to outreachy-kernel@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/CA%2BXqjF9B9dNXb0q2aZTeWMuLPSXh4mqi8LYNyxGvp1%3DvHw3HYQ%40mail.gmail.com.
> For more options, visit https://groups.google.com/d/optout.
>
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v6] staging: xgifb: correct the multiple line dereference
2017-02-28 8:29 ` Arushi Singhal
@ 2017-02-28 9:37 ` Greg Kroah-Hartman
2017-02-28 9:37 ` Greg Kroah-Hartman
1 sibling, 0 replies; 8+ messages in thread
From: Greg Kroah-Hartman @ 2017-02-28 9:37 UTC (permalink / raw)
To: Arushi Singhal; +Cc: arnaud.patard, devel, linux-kernel, outreachy-kernel
On Tue, Feb 28, 2017 at 01:59:53PM +0530, Arushi Singhal wrote:
Please fix your email client to not send html mail, otherwise the
mailing lists reject them, and your quoting is all messed up :(
> On Tue, Feb 28, 2017 at 11:21 AM, Greg Kroah-Hartman <
> gregkh@linuxfoundation.org> wrote:
>
> On Tue, Feb 28, 2017 at 10:35:30AM +0530, Arushi Singhal wrote:
> > Error reported by checkpatch.pl as "avoid multiple line dereference".
> > Addition of new variables to make the code more readable and also to
> > correct about mentioned error as by itroducing new variables line is
> > not exceeding 80 characters.
> >
> > Signed-off-by: Arushi Singhal <arushisinghal19971997@gmail.com>
> > ---
> > changes in v6
> >� �- changes done such that no other errors can generate.
> >� �- Improve the coding style.
> >� �- Introduced new variables.
> >� �- type of the variable is changed.
> >
> >� drivers/staging/xgifb/XGI_main_26.c | 29 ++++++-----------------------
> >� drivers/staging/xgifb/vb_setmode.c� | 17 +++++++++++------
> >� 2 files changed, 17 insertions(+), 29 deletions(-)
> >
> > diff --git a/drivers/staging/xgifb/XGI_main_26.c b/drivers/staging/xgifb/
> XGI_main_26.c
> > index 69ed137337ce..9870ea3b76b4 100644
> > --- a/drivers/staging/xgifb/XGI_main_26.c
> > +++ b/drivers/staging/xgifb/XGI_main_26.c
> > @@ -878,30 +878,13 @@ static void XGIfb_post_setmode(struct
> xgifb_video_info *xgifb_info)
> >� � � � � � � � � � � �}
> >
> >� � � � � � � � � � � �if ((filter >= 0) && (filter <= 7)) {
> > +� � � � � � � � � � � � � � �const u8 *f = XGI_TV_filter[filter_tb].
> filter[filter];
> >� � � � � � � � � � � � � � � �pr_debug("FilterTable[%d]-%d: %*ph\n",
> > -� � � � � � � � � � � � � � � � � � � filter_tb, filter,
> > -� � � � � � � � � � � � � � � � � � � 4, XGI_TV_filter[filter_tb].
> > -� � � � � � � � � � � � � � � � � � � � � � � � filter[filter]);
> > -� � � � � � � � � � � � � � �xgifb_reg_set(
> > -� � � � � � � � � � � � � � � � � � �XGIPART2,
> > -� � � � � � � � � � � � � � � � � � �0x35,
> > -� � � � � � � � � � � � � � � � � � �(XGI_TV_filter[filter_tb].
> > -� � � � � � � � � � � � � � � � � � � � � � �filter[filter][0]));
> > -� � � � � � � � � � � � � � �xgifb_reg_set(
> > -� � � � � � � � � � � � � � � � � � �XGIPART2,
> > -� � � � � � � � � � � � � � � � � � �0x36,
> > -� � � � � � � � � � � � � � � � � � �(XGI_TV_filter[filter_tb].
> > -� � � � � � � � � � � � � � � � � � � � � � �filter[filter][1]));
> > -� � � � � � � � � � � � � � �xgifb_reg_set(
> > -� � � � � � � � � � � � � � � � � � �XGIPART2,
> > -� � � � � � � � � � � � � � � � � � �0x37,
> > -� � � � � � � � � � � � � � � � � � �(XGI_TV_filter[filter_tb].
> > -� � � � � � � � � � � � � � � � � � � � � � �filter[filter][2]));
> > -� � � � � � � � � � � � � � �xgifb_reg_set(
> > -� � � � � � � � � � � � � � � � � � �XGIPART2,
> > -� � � � � � � � � � � � � � � � � � �0x38,
> > -� � � � � � � � � � � � � � � � � � �(XGI_TV_filter[filter_tb].
> > -� � � � � � � � � � � � � � � � � � � � � � �filter[filter][3]));
> > +� � � � � � � � � � � � � � � � � � � filter_tb, filter, 4, f);
> > +� � � � � � � � � � � � � � �xgifb_reg_set(XGIPART2, 0x35, f[0]);
> > +� � � � � � � � � � � � � � �xgifb_reg_set(XGIPART2, 0x36, f[1]);
> > +� � � � � � � � � � � � � � �xgifb_reg_set(XGIPART2, 0x37, f[2]);
> > +� � � � � � � � � � � � � � �xgifb_reg_set(XGIPART2, 0x38, f[3]);
> >� � � � � � � � � � � �}
> >� � � � � � � �}
> >� � � �}
> > diff --git a/drivers/staging/xgifb/vb_setmode.c b/drivers/staging/xgifb/
> vb_setmode.c
> > index 7c7c8c8f1df3..249a32804c06 100644
> > --- a/drivers/staging/xgifb/vb_setmode.c
> > +++ b/drivers/staging/xgifb/vb_setmode.c
> > @@ -221,8 +221,11 @@ static unsigned char XGI_AjustCRT2Rate(unsigned
> short ModeIdIndex,
> >
> >� � � �for (; XGI330_RefIndex[RefreshRateTableIndex + (*i)].ModeID ==
> >� � � � � � � tempbx; (*i)--) {
> > -� � � � � � �infoflag = XGI330_RefIndex[RefreshRateTableIndex + (*i)].
> > -� � � � � � � � � � � � � � �Ext_InfoFlag;
> > +� � � � � � �unsigned short j;
> > +
> > +� � � � � � �j = XGI330_RefIndex[RefreshRateTableIndex +
> (*i)].Ext_InfoFlag;
> > +� � � � � � �infoflag = j;
> > +
> >� � � � � � � �if (infoflag & tempax)
> >� � � � � � � � � � � �return 1;
>
>
> Why are you using a temporary variable 'j' here?� It's not needed at
> all, and just is confusing to read the code now, don't you agree?
>
> I am using temporary variable of small size(character) so that when
> I fixed the multiple line dereference then the line number of characters in a
> line will not increase by 80.
I know _why_ you are doing it, sorry, the point is that it makes no
sense at all to _do_ that. Please don't just blindly make random code
changes to shut the checkpatch.pl script up. You have to think, and use
it as a guide.
Remember, coding style changes are to be made to make the code easier to
read and understand by humans, not to quite a random perl script's
output.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v6] staging: xgifb: correct the multiple line dereference
@ 2017-02-28 9:37 ` Greg Kroah-Hartman
0 siblings, 0 replies; 8+ messages in thread
From: Greg Kroah-Hartman @ 2017-02-28 9:37 UTC (permalink / raw)
To: Arushi Singhal; +Cc: arnaud.patard, devel, linux-kernel, outreachy-kernel
On Tue, Feb 28, 2017 at 01:59:53PM +0530, Arushi Singhal wrote:
Please fix your email client to not send html mail, otherwise the
mailing lists reject them, and your quoting is all messed up :(
> On Tue, Feb 28, 2017 at 11:21 AM, Greg Kroah-Hartman <
> gregkh@linuxfoundation.org> wrote:
>
> On Tue, Feb 28, 2017 at 10:35:30AM +0530, Arushi Singhal wrote:
> > Error reported by checkpatch.pl as "avoid multiple line dereference".
> > Addition of new variables to make the code more readable and also to
> > correct about mentioned error as by itroducing new variables line is
> > not exceeding 80 characters.
> >
> > Signed-off-by: Arushi Singhal <arushisinghal19971997@gmail.com>
> > ---
> > changes in v6
> > - changes done such that no other errors can generate.
> > - Improve the coding style.
> > - Introduced new variables.
> > - type of the variable is changed.
> >
> > drivers/staging/xgifb/XGI_main_26.c | 29 ++++++-----------------------
> > drivers/staging/xgifb/vb_setmode.c | 17 +++++++++++------
> > 2 files changed, 17 insertions(+), 29 deletions(-)
> >
> > diff --git a/drivers/staging/xgifb/XGI_main_26.c b/drivers/staging/xgifb/
> XGI_main_26.c
> > index 69ed137337ce..9870ea3b76b4 100644
> > --- a/drivers/staging/xgifb/XGI_main_26.c
> > +++ b/drivers/staging/xgifb/XGI_main_26.c
> > @@ -878,30 +878,13 @@ static void XGIfb_post_setmode(struct
> xgifb_video_info *xgifb_info)
> > }
> >
> > if ((filter >= 0) && (filter <= 7)) {
> > + const u8 *f = XGI_TV_filter[filter_tb].
> filter[filter];
> > pr_debug("FilterTable[%d]-%d: %*ph\n",
> > - filter_tb, filter,
> > - 4, XGI_TV_filter[filter_tb].
> > - filter[filter]);
> > - xgifb_reg_set(
> > - XGIPART2,
> > - 0x35,
> > - (XGI_TV_filter[filter_tb].
> > - filter[filter][0]));
> > - xgifb_reg_set(
> > - XGIPART2,
> > - 0x36,
> > - (XGI_TV_filter[filter_tb].
> > - filter[filter][1]));
> > - xgifb_reg_set(
> > - XGIPART2,
> > - 0x37,
> > - (XGI_TV_filter[filter_tb].
> > - filter[filter][2]));
> > - xgifb_reg_set(
> > - XGIPART2,
> > - 0x38,
> > - (XGI_TV_filter[filter_tb].
> > - filter[filter][3]));
> > + filter_tb, filter, 4, f);
> > + xgifb_reg_set(XGIPART2, 0x35, f[0]);
> > + xgifb_reg_set(XGIPART2, 0x36, f[1]);
> > + xgifb_reg_set(XGIPART2, 0x37, f[2]);
> > + xgifb_reg_set(XGIPART2, 0x38, f[3]);
> > }
> > }
> > }
> > diff --git a/drivers/staging/xgifb/vb_setmode.c b/drivers/staging/xgifb/
> vb_setmode.c
> > index 7c7c8c8f1df3..249a32804c06 100644
> > --- a/drivers/staging/xgifb/vb_setmode.c
> > +++ b/drivers/staging/xgifb/vb_setmode.c
> > @@ -221,8 +221,11 @@ static unsigned char XGI_AjustCRT2Rate(unsigned
> short ModeIdIndex,
> >
> > for (; XGI330_RefIndex[RefreshRateTableIndex + (*i)].ModeID ==
> > tempbx; (*i)--) {
> > - infoflag = XGI330_RefIndex[RefreshRateTableIndex + (*i)].
> > - Ext_InfoFlag;
> > + unsigned short j;
> > +
> > + j = XGI330_RefIndex[RefreshRateTableIndex +
> (*i)].Ext_InfoFlag;
> > + infoflag = j;
> > +
> > if (infoflag & tempax)
> > return 1;
>
>
> Why are you using a temporary variable 'j' here? It's not needed at
> all, and just is confusing to read the code now, don't you agree?
>
> I am using temporary variable of small size(character) so that when
> I fixed the multiple line dereference then the line number of characters in a
> line will not increase by 80.
I know _why_ you are doing it, sorry, the point is that it makes no
sense at all to _do_ that. Please don't just blindly make random code
changes to shut the checkpatch.pl script up. You have to think, and use
it as a guide.
Remember, coding style changes are to be made to make the code easier to
read and understand by humans, not to quite a random perl script's
output.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v6] staging: xgifb: correct the multiple line dereference
2017-02-28 9:37 ` Greg Kroah-Hartman
(?)
@ 2017-02-28 10:16 ` Arushi Singhal
-1 siblings, 0 replies; 8+ messages in thread
From: Arushi Singhal @ 2017-02-28 10:16 UTC (permalink / raw)
To: Greg Kroah-Hartman; +Cc: arnaud.patard, devel, linux-kernel, outreachy-kernel
[-- Attachment #1: Type: text/plain, Size: 5736 bytes --]
Thanks Julia, Greg and Joe for always making it clear where I going wrong
and guiding me.
Thanks
Arushi
On Tue, Feb 28, 2017 at 3:07 PM, Greg Kroah-Hartman <
gregkh@linuxfoundation.org> wrote:
> On Tue, Feb 28, 2017 at 01:59:53PM +0530, Arushi Singhal wrote:
>
> Please fix your email client to not send html mail, otherwise the
> mailing lists reject them, and your quoting is all messed up :(
>
> > On Tue, Feb 28, 2017 at 11:21 AM, Greg Kroah-Hartman <
> > gregkh@linuxfoundation.org> wrote:
> >
> > On Tue, Feb 28, 2017 at 10:35:30AM +0530, Arushi Singhal wrote:
> > > Error reported by checkpatch.pl as "avoid multiple line
> dereference".
> > > Addition of new variables to make the code more readable and also
> to
> > > correct about mentioned error as by itroducing new variables line
> is
> > > not exceeding 80 characters.
> > >
> > > Signed-off-by: Arushi Singhal <arushisinghal19971997@gmail.com>
> > > ---
> > > changes in v6
> > > - changes done such that no other errors can generate.
> > > - Improve the coding style.
> > > - Introduced new variables.
> > > - type of the variable is changed.
> > >
> > > drivers/staging/xgifb/XGI_main_26.c | 29
> ++++++-----------------------
> > > drivers/staging/xgifb/vb_setmode.c | 17 +++++++++++------
> > > 2 files changed, 17 insertions(+), 29 deletions(-)
> > >
> > > diff --git a/drivers/staging/xgifb/XGI_main_26.c
> b/drivers/staging/xgifb/
> > XGI_main_26.c
> > > index 69ed137337ce..9870ea3b76b4 100644
> > > --- a/drivers/staging/xgifb/XGI_main_26.c
> > > +++ b/drivers/staging/xgifb/XGI_main_26.c
> > > @@ -878,30 +878,13 @@ static void XGIfb_post_setmode(struct
> > xgifb_video_info *xgifb_info)
> > > }
> > >
> > > if ((filter >= 0) && (filter <= 7)) {
> > > + const u8 *f =
> XGI_TV_filter[filter_tb].
> > filter[filter];
> > > pr_debug("FilterTable[%d]-%d:
> %*ph\n",
> > > - filter_tb, filter,
> > > - 4, XGI_TV_filter[filter_tb].
> > > - filter[filter]);
> > > - xgifb_reg_set(
> > > - XGIPART2,
> > > - 0x35,
> > > - (XGI_TV_filter[filter_tb].
> > > - filter[filter][0]));
> > > - xgifb_reg_set(
> > > - XGIPART2,
> > > - 0x36,
> > > - (XGI_TV_filter[filter_tb].
> > > - filter[filter][1]));
> > > - xgifb_reg_set(
> > > - XGIPART2,
> > > - 0x37,
> > > - (XGI_TV_filter[filter_tb].
> > > - filter[filter][2]));
> > > - xgifb_reg_set(
> > > - XGIPART2,
> > > - 0x38,
> > > - (XGI_TV_filter[filter_tb].
> > > - filter[filter][3]));
> > > + filter_tb, filter, 4, f);
> > > + xgifb_reg_set(XGIPART2, 0x35, f[0]);
> > > + xgifb_reg_set(XGIPART2, 0x36, f[1]);
> > > + xgifb_reg_set(XGIPART2, 0x37, f[2]);
> > > + xgifb_reg_set(XGIPART2, 0x38, f[3]);
> > > }
> > > }
> > > }
> > > diff --git a/drivers/staging/xgifb/vb_setmode.c
> b/drivers/staging/xgifb/
> > vb_setmode.c
> > > index 7c7c8c8f1df3..249a32804c06 100644
> > > --- a/drivers/staging/xgifb/vb_setmode.c
> > > +++ b/drivers/staging/xgifb/vb_setmode.c
> > > @@ -221,8 +221,11 @@ static unsigned char
> XGI_AjustCRT2Rate(unsigned
> > short ModeIdIndex,
> > >
> > > for (; XGI330_RefIndex[RefreshRateTableIndex + (*i)].ModeID
> ==
> > > tempbx; (*i)--) {
> > > - infoflag = XGI330_RefIndex[RefreshRateTableIndex +
> (*i)].
> > > - Ext_InfoFlag;
> > > + unsigned short j;
> > > +
> > > + j = XGI330_RefIndex[RefreshRateTableIndex +
> > (*i)].Ext_InfoFlag;
> > > + infoflag = j;
> > > +
> > > if (infoflag & tempax)
> > > return 1;
> >
> >
> > Why are you using a temporary variable 'j' here? It's not needed at
> > all, and just is confusing to read the code now, don't you agree?
> >
> > I am using temporary variable of small size(character) so that when
> > I fixed the multiple line dereference then the line number of characters
> in a
> > line will not increase by 80.
>
> I know _why_ you are doing it, sorry, the point is that it makes no
> sense at all to _do_ that. Please don't just blindly make random code
> changes to shut the checkpatch.pl script up. You have to think, and use
> it as a guide.
>
> Remember, coding style changes are to be made to make the code easier to
> read and understand by humans, not to quite a random perl script's
> output.
>
> thanks,
>
> greg k-h
>
[-- Attachment #2: Type: text/html, Size: 8244 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2017-02-28 10:31 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-02-28 5:05 [PATCH v6] staging: xgifb: correct the multiple line dereference Arushi Singhal
2017-02-28 5:51 ` Greg Kroah-Hartman
2017-02-28 6:19 ` Joe Perches
2017-02-28 8:29 ` Arushi Singhal
2017-02-28 8:44 ` [Outreachy kernel] " Julia Lawall
2017-02-28 9:37 ` Greg Kroah-Hartman
2017-02-28 9:37 ` Greg Kroah-Hartman
2017-02-28 10:16 ` Arushi Singhal
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.