* [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; 5+ 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] 5+ 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
[not found] ` <CA+XqjF9B9dNXb0q2aZTeWMuLPSXh4mqi8LYNyxGvp1=vHw3HYQ@mail.gmail.com>
0 siblings, 2 replies; 5+ 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] 5+ 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
[not found] ` <CA+XqjF9B9dNXb0q2aZTeWMuLPSXh4mqi8LYNyxGvp1=vHw3HYQ@mail.gmail.com>
1 sibling, 0 replies; 5+ 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] 5+ messages in thread
* Re: [Outreachy kernel] Re: [PATCH v6] staging: xgifb: correct the multiple line dereference
[not found] ` <CA+XqjF9B9dNXb0q2aZTeWMuLPSXh4mqi8LYNyxGvp1=vHw3HYQ@mail.gmail.com>
@ 2017-02-28 8:44 ` Julia Lawall
2017-02-28 9:37 ` Greg Kroah-Hartman
1 sibling, 0 replies; 5+ 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] 5+ messages in thread
* Re: [PATCH v6] staging: xgifb: correct the multiple line dereference
[not found] ` <CA+XqjF9B9dNXb0q2aZTeWMuLPSXh4mqi8LYNyxGvp1=vHw3HYQ@mail.gmail.com>
2017-02-28 8:44 ` [Outreachy kernel] " Julia Lawall
@ 2017-02-28 9:37 ` Greg Kroah-Hartman
1 sibling, 0 replies; 5+ 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] 5+ messages in thread
end of thread, other threads:[~2017-02-28 10:31 UTC | newest]
Thread overview: 5+ 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
[not found] ` <CA+XqjF9B9dNXb0q2aZTeWMuLPSXh4mqi8LYNyxGvp1=vHw3HYQ@mail.gmail.com>
2017-02-28 8:44 ` [Outreachy kernel] " Julia Lawall
2017-02-28 9:37 ` Greg Kroah-Hartman
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).