* [PATCH] staging: sm750fb: make pointers in array const @ 2021-10-18 16:44 Kushal Kothari 2021-10-18 16:54 ` [Outreachy kernel] " Fabio M. De Francesco 0 siblings, 1 reply; 13+ messages in thread From: Kushal Kothari @ 2021-10-18 16:44 UTC (permalink / raw) To: sudipm.mukherjee, teddy.wang, gregkh, outreachy-kernel, mike.rapoport, kushalkothari2850 Cc: Kushal Kothari Change the parameters of functions from const char *g_fbmode[] to const char * const g_fbmode[]. This additional const is needed to allow us to fix checkpatch warning, as well as being good programming practice. For the checkpatch warnings, if we have a set of command line args that we want to check defined as: static const char * g_fbmode[] = {NULL, NULL}; checkpatch will complain: WARNING: static const char * array should probably be static const char * const Signed-off-by: Kushal Kothari <kushalkothari285@gmail.com> --- drivers/staging/sm750fb/sm750.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/staging/sm750fb/sm750.c b/drivers/staging/sm750fb/sm750.c index dbd1159a2ef0..3d9b4b0efcb1 100644 --- a/drivers/staging/sm750fb/sm750.c +++ b/drivers/staging/sm750fb/sm750.c @@ -33,7 +33,7 @@ static int g_hwcursor = 1; static int g_noaccel; static int g_nomtrr; -static const char *g_fbmode[] = {NULL, NULL}; +static const char * const g_fbmode[] = {NULL, NULL}; static const char *g_def_fbmode = "1024x768-32@60"; static char *g_settings; static int g_dualview; -- 2.25.1 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [Outreachy kernel] [PATCH] staging: sm750fb: make pointers in array const 2021-10-18 16:44 [PATCH] staging: sm750fb: make pointers in array const Kushal Kothari @ 2021-10-18 16:54 ` Fabio M. De Francesco 2021-10-18 17:01 ` Julia Lawall 2021-10-18 18:18 ` Fabio M. De Francesco 0 siblings, 2 replies; 13+ messages in thread From: Fabio M. De Francesco @ 2021-10-18 16:54 UTC (permalink / raw) To: sudipm.mukherjee, teddy.wang, gregkh, outreachy-kernel, mike.rapoport, kushalkothari2850 Cc: Kushal Kothari, Kushal Kothari On Monday, October 18, 2021 6:44:31 PM CEST Kushal Kothari wrote: > Change the parameters of functions from const char *g_fbmode[] to > const char * const g_fbmode[]. This additional const is needed to > allow us to fix checkpatch warning, as well as being good > programming practice. > > For the checkpatch warnings, if we have a set of command line > args that we want to check defined as: > static const char * g_fbmode[] = {NULL, NULL}; > > checkpatch will complain: > WARNING: static const char * array should probably be static const char * const > > Signed-off-by: Kushal Kothari <kushalkothari285@gmail.com> > --- > drivers/staging/sm750fb/sm750.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/staging/sm750fb/sm750.c b/drivers/staging/sm750fb/ sm750.c > index dbd1159a2ef0..3d9b4b0efcb1 100644 > --- a/drivers/staging/sm750fb/sm750.c > +++ b/drivers/staging/sm750fb/sm750.c > @@ -33,7 +33,7 @@ > static int g_hwcursor = 1; > static int g_noaccel; > static int g_nomtrr; > -static const char *g_fbmode[] = {NULL, NULL}; > +static const char * const g_fbmode[] = {NULL, NULL}; You have introduced a logical change (g_fbmode[] entries cannot be assigned any more) and a build error (because there is code somewhere that assigns values to those slots). Please, build the code after you've changed it. Thanks, Fabio > static const char *g_def_fbmode = "1024x768-32@60"; > static char *g_settings; > static int g_dualview; > -- > 2.25.1 > > -- > 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 view this discussion on the web visit https://groups.google.com/d/msgid/ outreachy-kernel/20211018164431.26462-1-kushalkothari285%40gmail.com. > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Outreachy kernel] [PATCH] staging: sm750fb: make pointers in array const 2021-10-18 16:54 ` [Outreachy kernel] " Fabio M. De Francesco @ 2021-10-18 17:01 ` Julia Lawall 2021-10-18 17:18 ` Fabio M. De Francesco 2021-10-18 18:18 ` Fabio M. De Francesco 1 sibling, 1 reply; 13+ messages in thread From: Julia Lawall @ 2021-10-18 17:01 UTC (permalink / raw) To: Fabio M. De Francesco Cc: sudipm.mukherjee, teddy.wang, gregkh, outreachy-kernel, mike.rapoport, kushalkothari2850, Kushal Kothari, Kushal Kothari, Joe Perches On Mon, 18 Oct 2021, Fabio M. De Francesco wrote: > On Monday, October 18, 2021 6:44:31 PM CEST Kushal Kothari wrote: > > Change the parameters of functions from const char *g_fbmode[] to > > const char * const g_fbmode[]. This additional const is needed to > > allow us to fix checkpatch warning, as well as being good > > programming practice. > > > > For the checkpatch warnings, if we have a set of command line > > args that we want to check defined as: > > static const char * g_fbmode[] = {NULL, NULL}; > > > > checkpatch will complain: > > WARNING: static const char * array should probably be static const > char * const > > > > Signed-off-by: Kushal Kothari <kushalkothari285@gmail.com> > > --- > > drivers/staging/sm750fb/sm750.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/staging/sm750fb/sm750.c b/drivers/staging/sm750fb/ > sm750.c > > index dbd1159a2ef0..3d9b4b0efcb1 100644 > > --- a/drivers/staging/sm750fb/sm750.c > > +++ b/drivers/staging/sm750fb/sm750.c > > @@ -33,7 +33,7 @@ > > static int g_hwcursor = 1; > > static int g_noaccel; > > static int g_nomtrr; > > -static const char *g_fbmode[] = {NULL, NULL}; > > +static const char * const g_fbmode[] = {NULL, NULL}; > > You have introduced a logical change (g_fbmode[] entries cannot be assigned > any more) and a build error (because there is code somewhere that assigns > values to those slots). I wonder if this warning makes much sense when the array elements are NULL. I don't know if checkpatch could easily detect that. julia > > Please, build the code after you've changed it. > > Thanks, > > Fabio > > > static const char *g_def_fbmode = "1024x768-32@60"; > > static char *g_settings; > > static int g_dualview; > > -- > > 2.25.1 > > > > -- > > 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 view this discussion on the web visit https://groups.google.com/d/msgid/ > outreachy-kernel/20211018164431.26462-1-kushalkothari285%40gmail.com. > > > > > > > -- > 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 view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/163460335.XzdQGyDEVH%40localhost.localdomain. > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Outreachy kernel] [PATCH] staging: sm750fb: make pointers in array const 2021-10-18 17:01 ` Julia Lawall @ 2021-10-18 17:18 ` Fabio M. De Francesco 2021-10-18 18:22 ` Joe Perches 0 siblings, 1 reply; 13+ messages in thread From: Fabio M. De Francesco @ 2021-10-18 17:18 UTC (permalink / raw) To: Julia Lawall Cc: sudipm.mukherjee, teddy.wang, gregkh, outreachy-kernel, mike.rapoport, kushalkothari2850, Kushal Kothari, Kushal Kothari, Joe Perches On Monday, October 18, 2021 7:01:42 PM CEST Julia Lawall wrote: > > On Mon, 18 Oct 2021, Fabio M. De Francesco wrote: > > > On Monday, October 18, 2021 6:44:31 PM CEST Kushal Kothari wrote: > > > Change the parameters of functions from const char *g_fbmode[] to > > > const char * const g_fbmode[]. This additional const is needed to > > > allow us to fix checkpatch warning, as well as being good > > > programming practice. > > > > > > For the checkpatch warnings, if we have a set of command line > > > args that we want to check defined as: > > > static const char * g_fbmode[] = {NULL, NULL}; > > > > > > checkpatch will complain: > > > WARNING: static const char * array should probably be static const > > char * const > > > > > > Signed-off-by: Kushal Kothari <kushalkothari285@gmail.com> > > > --- > > > > > > [...] > > > > > > -static const char *g_fbmode[] = {NULL, NULL}; > > > +static const char * const g_fbmode[] = {NULL, NULL}; > > > > You have introduced a logical change (g_fbmode[] entries cannot be assigned > > any more) and a build error (because there is code somewhere that assigns > > values to those slots). > > I wonder if this warning makes much sense when the array elements are > NULL. I don't know if checkpatch could easily detect that. > > julia Unfortunately I cannot change checkpatch.pl because I am not able to program with Perl. However, it should be an easy fix in whatever programming language: checkpatch should warn if and only if the array elements are assigned with non 'NULL' values, because it's pretty clear that somewhere else there must be some lines that assign values to them. Fabio > > > > > Please, build the code after you've changed it. > > > > Thanks, > > > > Fabio > > > > > static const char *g_def_fbmode = "1024x768-32@60"; > > > static char *g_settings; > > > static int g_dualview; > > > -- > > > 2.25.1 > > > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Outreachy kernel] [PATCH] staging: sm750fb: make pointers in array const 2021-10-18 17:18 ` Fabio M. De Francesco @ 2021-10-18 18:22 ` Joe Perches 2021-10-18 18:34 ` Fabio M. De Francesco 0 siblings, 1 reply; 13+ messages in thread From: Joe Perches @ 2021-10-18 18:22 UTC (permalink / raw) To: Fabio M. De Francesco, Julia Lawall Cc: sudipm.mukherjee, teddy.wang, gregkh, outreachy-kernel, mike.rapoport, kushalkothari2850, Kushal Kothari On Mon, 2021-10-18 at 19:18 +0200, Fabio M. De Francesco wrote: > On Monday, October 18, 2021 7:01:42 PM CEST Julia Lawall wrote: > > > > On Mon, 18 Oct 2021, Fabio M. De Francesco wrote: > > > > > On Monday, October 18, 2021 6:44:31 PM CEST Kushal Kothari wrote: > > > > Change the parameters of functions from const char *g_fbmode[] to > > > > const char * const g_fbmode[]. This additional const is needed to > > > > allow us to fix checkpatch warning, as well as being good > > > > programming practice. > > > > > > > > For the checkpatch warnings, if we have a set of command line > > > > args that we want to check defined as: > > > > static const char * g_fbmode[] = {NULL, NULL}; > > > > > > > > checkpatch will complain: > > > > WARNING: static const char * array should probably be static > const > > > char * const > > > > > > > > Signed-off-by: Kushal Kothari <kushalkothari285@gmail.com> > > > > --- > > > > > > > > [...] > > > > > > > > -static const char *g_fbmode[] = {NULL, NULL}; > > > > +static const char * const g_fbmode[] = {NULL, NULL}; > > > > > > You have introduced a logical change (g_fbmode[] entries cannot be > assigned > > > any more) and a build error (because there is code somewhere that assigns > > > values to those slots). > > > > I wonder if this warning makes much sense when the array elements are > > NULL. I don't know if checkpatch could easily detect that. No, it couldn't really. It's a line by line parser and most frequently these are on separate lines. > However, it should be an easy fix in whatever programming language: > checkpatch should warn if and only if the array elements are assigned with > non 'NULL' values, because it's pretty clear that somewhere else there must > be some lines that assign values to them. Don't take checkpatch warning too seriously and do compile and test any change you make _before_ you submit a patch. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Outreachy kernel] [PATCH] staging: sm750fb: make pointers in array const 2021-10-18 18:22 ` Joe Perches @ 2021-10-18 18:34 ` Fabio M. De Francesco 2021-10-18 18:43 ` Julia Lawall 2021-10-18 18:52 ` Joe Perches 0 siblings, 2 replies; 13+ messages in thread From: Fabio M. De Francesco @ 2021-10-18 18:34 UTC (permalink / raw) To: Julia Lawall, Joe Perches Cc: sudipm.mukherjee, teddy.wang, gregkh, outreachy-kernel, mike.rapoport, kushalkothari2850, Kushal Kothari On Monday, October 18, 2021 8:22:23 PM CEST Joe Perches wrote: > On Mon, 2021-10-18 at 19:18 +0200, Fabio M. De Francesco wrote: > > On Monday, October 18, 2021 7:01:42 PM CEST Julia Lawall wrote: > > > > > > On Mon, 18 Oct 2021, Fabio M. De Francesco wrote: > > > > > > > On Monday, October 18, 2021 6:44:31 PM CEST Kushal Kothari wrote: > > > > > Change the parameters of functions from const char *g_fbmode[] to > > > > > const char * const g_fbmode[]. This additional const is needed to > > > > > allow us to fix checkpatch warning, as well as being good > > > > > programming practice. > > > > > > > > > > For the checkpatch warnings, if we have a set of command line > > > > > args that we want to check defined as: > > > > > static const char * g_fbmode[] = {NULL, NULL}; > > > > > > > > > > checkpatch will complain: > > > > > WARNING: static const char * array should probably be static > > const > > > > char * const > > > > > > > > > > Signed-off-by: Kushal Kothari <kushalkothari285@gmail.com> > > > > > --- > > > > > > > > > > [...] > > > > > > > > > > -static const char *g_fbmode[] = {NULL, NULL}; > > > > > +static const char * const g_fbmode[] = {NULL, NULL}; > > > > > > > > You have introduced a logical change (g_fbmode[] entries cannot be > > assigned > > > > any more) and a build error (because there is code somewhere that assigns > > > > values to those slots). > > > > > > I wonder if this warning makes much sense when the array elements are > > > NULL. I don't know if checkpatch could easily detect that. > > No, it couldn't really. It's a line by line parser and most frequently > these are on separate lines. Sorry but I don't get it. For sure I'm missing something... I guess that checkpatch.pl warned Kushal because it detected that the array of pointers was assigned with something and so it output that "static const char * array should probably be static const". What I cannot understand is why it _can_ detect that the array is assigned with some values but it _cannot_ check that those values are 'NULL' and so avoid to warn. Thanks, Fabio > > > However, it should be an easy fix in whatever programming language: > > checkpatch should warn if and only if the array elements are assigned with > > non 'NULL' values, because it's pretty clear that somewhere else there must > > be some lines that assign values to them. > > Don't take checkpatch warning too seriously and do compile > and test any change you make _before_ you submit a patch. > > > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Outreachy kernel] [PATCH] staging: sm750fb: make pointers in array const 2021-10-18 18:34 ` Fabio M. De Francesco @ 2021-10-18 18:43 ` Julia Lawall 2021-10-18 18:56 ` kushal kothari 2021-10-18 18:56 ` Fabio M. De Francesco 2021-10-18 18:52 ` Joe Perches 1 sibling, 2 replies; 13+ messages in thread From: Julia Lawall @ 2021-10-18 18:43 UTC (permalink / raw) To: Fabio M. De Francesco Cc: Joe Perches, sudipm.mukherjee, teddy.wang, gregkh, outreachy-kernel, mike.rapoport, kushalkothari2850, Kushal Kothari On Mon, 18 Oct 2021, Fabio M. De Francesco wrote: > On Monday, October 18, 2021 8:22:23 PM CEST Joe Perches wrote: > > On Mon, 2021-10-18 at 19:18 +0200, Fabio M. De Francesco wrote: > > > On Monday, October 18, 2021 7:01:42 PM CEST Julia Lawall wrote: > > > > > > > > On Mon, 18 Oct 2021, Fabio M. De Francesco wrote: > > > > > > > > > On Monday, October 18, 2021 6:44:31 PM CEST Kushal Kothari wrote: > > > > > > Change the parameters of functions from const char *g_fbmode[] to > > > > > > const char * const g_fbmode[]. This additional const is needed to > > > > > > allow us to fix checkpatch warning, as well as being good > > > > > > programming practice. > > > > > > > > > > > > For the checkpatch warnings, if we have a set of command line > > > > > > args that we want to check defined as: > > > > > > static const char * g_fbmode[] = {NULL, NULL}; > > > > > > > > > > > > checkpatch will complain: > > > > > > WARNING: static const char * array should probably be static > > > const > > > > > char * const > > > > > > > > > > > > Signed-off-by: Kushal Kothari <kushalkothari285@gmail.com> > > > > > > --- > > > > > > > > > > > > [...] > > > > > > > > > > > > -static const char *g_fbmode[] = {NULL, NULL}; > > > > > > +static const char * const g_fbmode[] = {NULL, NULL}; > > > > > > > > > > You have introduced a logical change (g_fbmode[] entries cannot be > > > assigned > > > > > any more) and a build error (because there is code somewhere that > assigns > > > > > values to those slots). > > > > > > > > I wonder if this warning makes much sense when the array elements are > > > > NULL. I don't know if checkpatch could easily detect that. > > > > No, it couldn't really. It's a line by line parser and most frequently > > these are on separate lines. > > Sorry but I don't get it. For sure I'm missing something... > > I guess that checkpatch.pl warned Kushal because it detected that the array > of pointers was assigned with something and so it output that "static const > char * array should probably be static const". > > What I cannot understand is why it _can_ detect that the array is assigned > with some values but it _cannot_ check that those values are 'NULL' and so > avoid to warn. In this case it could, but this case is too specific to be worth making a special case for. The problem is that checkpatch only looks at one line at a time, so some values could be on another line. julia > > Thanks, > > Fabio > > > > > > However, it should be an easy fix in whatever programming language: > > > checkpatch should warn if and only if the array elements are assigned > with > > > non 'NULL' values, because it's pretty clear that somewhere else there > must > > > be some lines that assign values to them. > > > > Don't take checkpatch warning too seriously and do compile > > and test any change you make _before_ you submit a patch. > > > > > > > > > > > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Outreachy kernel] [PATCH] staging: sm750fb: make pointers in array const 2021-10-18 18:43 ` Julia Lawall @ 2021-10-18 18:56 ` kushal kothari 2021-10-18 19:53 ` Alison Schofield 2021-10-18 18:56 ` Fabio M. De Francesco 1 sibling, 1 reply; 13+ messages in thread From: kushal kothari @ 2021-10-18 18:56 UTC (permalink / raw) To: outreachy-kernel [-- Attachment #1.1: Type: text/plain, Size: 3599 bytes --] >As I wrote your patch has errors, however, if it had no errors it could not >be taken in any case. I just noticed that you forgot to CC linux- > <https://groups.google.com/>staging@lists.linux.dev. >Some days ago, Greg Kroah-Hartman wrote clearly that he cannot take patches >that are not submitted to linux-staging. Yes, I forgot and CC'ed only to the output of the list of that script . I'll keep this in mind ahead. Thanks. On Tuesday, October 19, 2021 at 12:13:47 AM UTC+5:30 Julia....@inria.fr wrote: > > > On Mon, 18 Oct 2021, Fabio M. De Francesco wrote: > > > On Monday, October 18, 2021 8:22:23 PM CEST Joe Perches wrote: > > > On Mon, 2021-10-18 at 19:18 +0200, Fabio M. De Francesco wrote: > > > > On Monday, October 18, 2021 7:01:42 PM CEST Julia Lawall wrote: > > > > > > > > > > On Mon, 18 Oct 2021, Fabio M. De Francesco wrote: > > > > > > > > > > > On Monday, October 18, 2021 6:44:31 PM CEST Kushal Kothari wrote: > > > > > > > Change the parameters of functions from const char *g_fbmode[] > to > > > > > > > const char * const g_fbmode[]. This additional const is needed > to > > > > > > > allow us to fix checkpatch warning, as well as being good > > > > > > > programming practice. > > > > > > > > > > > > > > For the checkpatch warnings, if we have a set of command line > > > > > > > args that we want to check defined as: > > > > > > > static const char * g_fbmode[] = {NULL, NULL}; > > > > > > > > > > > > > > checkpatch will complain: > > > > > > > WARNING: static const char * array should probably be static > > > > const > > > > > > char * const > > > > > > > > > > > > > > Signed-off-by: Kushal Kothari <kushalko...@gmail.com> > > > > > > > --- > > > > > > > > > > > > > > [...] > > > > > > > > > > > > > > -static const char *g_fbmode[] = {NULL, NULL}; > > > > > > > +static const char * const g_fbmode[] = {NULL, NULL}; > > > > > > > > > > > > You have introduced a logical change (g_fbmode[] entries cannot > be > > > > assigned > > > > > > any more) and a build error (because there is code somewhere that > > assigns > > > > > > values to those slots). > > > > > > > > > > I wonder if this warning makes much sense when the array elements > are > > > > > NULL. I don't know if checkpatch could easily detect that. > > > > > > No, it couldn't really. It's a line by line parser and most frequently > > > these are on separate lines. > > > > Sorry but I don't get it. For sure I'm missing something... > > > > I guess that checkpatch.pl warned Kushal because it detected that the > array > > of pointers was assigned with something and so it output that "static > const > > char * array should probably be static const". > > > > What I cannot understand is why it _can_ detect that the array is > assigned > > with some values but it _cannot_ check that those values are 'NULL' and > so > > avoid to warn. > > In this case it could, but this case is too specific to be worth making a > special case for. The problem is that checkpatch only looks at one line > at a time, so some values could be on another line. > > julia > > > > > Thanks, > > > > Fabio > > > > > > > > > However, it should be an easy fix in whatever programming language: > > > > checkpatch should warn if and only if the array elements are assigned > > with > > > > non 'NULL' values, because it's pretty clear that somewhere else > there > > must > > > > be some lines that assign values to them. > > > > > > Don't take checkpatch warning too seriously and do compile > > > and test any change you make _before_ you submit a patch. > > > > > > > > > > > > > > > > > > > > [-- Attachment #1.2: Type: text/html, Size: 5151 bytes --] ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Outreachy kernel] [PATCH] staging: sm750fb: make pointers in array const 2021-10-18 18:56 ` kushal kothari @ 2021-10-18 19:53 ` Alison Schofield 0 siblings, 0 replies; 13+ messages in thread From: Alison Schofield @ 2021-10-18 19:53 UTC (permalink / raw) To: kushal kothari; +Cc: outreachy-kernel On Mon, Oct 18, 2021 at 11:56:30AM -0700, kushal kothari wrote: > >As I wrote your patch has errors, however, if it had no errors it could > not > >be taken in any case. I just noticed that you forgot to CC linux- > > <https://groups.google.com/>staging@lists.linux.dev. > > >Some days ago, Greg Kroah-Hartman wrote clearly that he cannot take > patches > >that are not submitted to linux-staging. > > Yes, I forgot and CC'ed only to the output of the list of that script . > I'll keep this in mind ahead. > Thanks. Hi Kushal, Thanks for your perserverance. I'm not sure this is mentioned anywhere in the first patch tutorial, but is should be: avoid pruning the recipient list in an ongoing email thread. I wanted to reply to this, but I see the recipient list got truncated along the way. Adding recipients, but pruning in an ongoing discussion gets confusing. wrt who to send the patches to - you should not be pruning that list either. Read back through previous emails on this list about that. The send to Greg is not a special request. It's default behavior when get_maintainers is used correctly. Thanks, Alison > > On Tuesday, October 19, 2021 at 12:13:47 AM UTC+5:30 Julia....@inria.fr > wrote: > > > > > > > On Mon, 18 Oct 2021, Fabio M. De Francesco wrote: > > > > > On Monday, October 18, 2021 8:22:23 PM CEST Joe Perches wrote: > > > > On Mon, 2021-10-18 at 19:18 +0200, Fabio M. De Francesco wrote: > > > > > On Monday, October 18, 2021 7:01:42 PM CEST Julia Lawall wrote: > > > > > > > > > > > > On Mon, 18 Oct 2021, Fabio M. De Francesco wrote: > > > > > > > > > > > > > On Monday, October 18, 2021 6:44:31 PM CEST Kushal Kothari wrote: > > > > > > > > Change the parameters of functions from const char *g_fbmode[] > > to > > > > > > > > const char * const g_fbmode[]. This additional const is needed > > to > > > > > > > > allow us to fix checkpatch warning, as well as being good > > > > > > > > programming practice. > > > > > > > > > > > > > > > > For the checkpatch warnings, if we have a set of command line > > > > > > > > args that we want to check defined as: > > > > > > > > static const char * g_fbmode[] = {NULL, NULL}; > > > > > > > > > > > > > > > > checkpatch will complain: > > > > > > > > WARNING: static const char * array should probably be static > > > > > const > > > > > > > char * const > > > > > > > > > > > > > > > > Signed-off-by: Kushal Kothari <kushalko...@gmail.com> > > > > > > > > --- > > > > > > > > > > > > > > > > [...] > > > > > > > > > > > > > > > > -static const char *g_fbmode[] = {NULL, NULL}; > > > > > > > > +static const char * const g_fbmode[] = {NULL, NULL}; > > > > > > > > > > > > > > You have introduced a logical change (g_fbmode[] entries cannot > > be > > > > > assigned > > > > > > > any more) and a build error (because there is code somewhere that > > > assigns > > > > > > > values to those slots). > > > > > > > > > > > > I wonder if this warning makes much sense when the array elements > > are > > > > > > NULL. I don't know if checkpatch could easily detect that. > > > > > > > > No, it couldn't really. It's a line by line parser and most frequently > > > > these are on separate lines. > > > > > > Sorry but I don't get it. For sure I'm missing something... > > > > > > I guess that checkpatch.pl warned Kushal because it detected that the > > array > > > of pointers was assigned with something and so it output that "static > > const > > > char * array should probably be static const". > > > > > > What I cannot understand is why it _can_ detect that the array is > > assigned > > > with some values but it _cannot_ check that those values are 'NULL' and > > so > > > avoid to warn. > > > > In this case it could, but this case is too specific to be worth making a > > special case for. The problem is that checkpatch only looks at one line > > at a time, so some values could be on another line. > > > > julia > > > > > > > > Thanks, > > > > > > Fabio > > > > > > > > > > > > However, it should be an easy fix in whatever programming language: > > > > > checkpatch should warn if and only if the array elements are assigned > > > with > > > > > non 'NULL' values, because it's pretty clear that somewhere else > > there > > > must > > > > > be some lines that assign values to them. > > > > > > > > Don't take checkpatch warning too seriously and do compile > > > > and test any change you make _before_ you submit a patch. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > -- > 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 view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/cc951a48-8d3f-4243-8dc6-bae3446941c1n%40googlegroups.com. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Outreachy kernel] [PATCH] staging: sm750fb: make pointers in array const 2021-10-18 18:43 ` Julia Lawall 2021-10-18 18:56 ` kushal kothari @ 2021-10-18 18:56 ` Fabio M. De Francesco 1 sibling, 0 replies; 13+ messages in thread From: Fabio M. De Francesco @ 2021-10-18 18:56 UTC (permalink / raw) To: Julia Lawall Cc: Joe Perches, sudipm.mukherjee, teddy.wang, gregkh, outreachy-kernel, mike.rapoport, kushalkothari2850, Kushal Kothari On Monday, October 18, 2021 8:43:44 PM CEST Julia Lawall wrote: > > On Mon, 18 Oct 2021, Fabio M. De Francesco wrote: > > > On Monday, October 18, 2021 8:22:23 PM CEST Joe Perches wrote: > > > On Mon, 2021-10-18 at 19:18 +0200, Fabio M. De Francesco wrote: > > > > On Monday, October 18, 2021 7:01:42 PM CEST Julia Lawall wrote: > > > > > > > > > > On Mon, 18 Oct 2021, Fabio M. De Francesco wrote: > > > > > > > > > > > On Monday, October 18, 2021 6:44:31 PM CEST Kushal Kothari wrote: > > > > > > > Change the parameters of functions from const char *g_fbmode[] to > > > > > > > const char * const g_fbmode[]. This additional const is needed to > > > > > > > allow us to fix checkpatch warning, as well as being good > > > > > > > programming practice. > > > > > > > > > > > > > > For the checkpatch warnings, if we have a set of command line > > > > > > > args that we want to check defined as: > > > > > > > static const char * g_fbmode[] = {NULL, NULL}; > > > > > > > > > > > > > > checkpatch will complain: > > > > > > > WARNING: static const char * array should probably be static > > > > const > > > > > > char * const > > > > > > > > > > > > > > Signed-off-by: Kushal Kothari <kushalkothari285@gmail.com> > > > > > > > --- > > > > > > > > > > > > > > [...] > > > > > > > > > > > > > > -static const char *g_fbmode[] = {NULL, NULL}; > > > > > > > +static const char * const g_fbmode[] = {NULL, NULL}; > > > > > > > > > > > > You have introduced a logical change (g_fbmode[] entries cannot be > > > > assigned > > > > > > any more) and a build error (because there is code somewhere that > > assigns > > > > > > values to those slots). > > > > > > > > > > I wonder if this warning makes much sense when the array elements are > > > > > NULL. I don't know if checkpatch could easily detect that. > > > > > > No, it couldn't really. It's a line by line parser and most frequently > > > these are on separate lines. > > > > Sorry but I don't get it. For sure I'm missing something... > > > > I guess that checkpatch.pl warned Kushal because it detected that the array > > of pointers was assigned with something and so it output that "static const > > char * array should probably be static const". > > > > What I cannot understand is why it _can_ detect that the array is assigned > > with some values but it _cannot_ check that those values are 'NULL' and so > > avoid to warn. > > In this case it could, but this case is too specific to be worth making a > special case for. The problem is that checkpatch only looks at one line > at a time, so some values could be on another line. Yes, I understand that checkpatch's developers want to keep it simple and check one line at a time. After all, as Joe wrote, this is just a tool and final decisions should be made by the users of the tool. I'm not trying to justify errors like the one that Kushal made by saying that checkpatch is the culprit. I've seen hundreds of warning output by static analyzers that are mere false positives. I guess that entering a loop and checking line by line until it gets to the closing braces (or to a value != NULL) is not something that is worth doing. Thanks, Fabio > > julia > > > > > Thanks, > > > > Fabio > > > > > > > > > However, it should be an easy fix in whatever programming language: > > > > checkpatch should warn if and only if the array elements are assigned > > with > > > > non 'NULL' values, because it's pretty clear that somewhere else there > > must > > > > be some lines that assign values to them. > > > > > > Don't take checkpatch warning too seriously and do compile > > > and test any change you make _before_ you submit a patch. > > > > > > > > > > > > > > > > > > > > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Outreachy kernel] [PATCH] staging: sm750fb: make pointers in array const 2021-10-18 18:34 ` Fabio M. De Francesco 2021-10-18 18:43 ` Julia Lawall @ 2021-10-18 18:52 ` Joe Perches 2021-10-18 19:01 ` Fabio M. De Francesco 1 sibling, 1 reply; 13+ messages in thread From: Joe Perches @ 2021-10-18 18:52 UTC (permalink / raw) To: Fabio M. De Francesco, Julia Lawall Cc: sudipm.mukherjee, teddy.wang, gregkh, outreachy-kernel, mike.rapoport, kushalkothari2850, Kushal Kothari On Mon, 2021-10-18 at 20:34 +0200, Fabio M. De Francesco wrote: > On Monday, October 18, 2021 8:22:23 PM CEST Joe Perches wrote: > > On Mon, 2021-10-18 at 19:18 +0200, Fabio M. De Francesco wrote: > > > On Monday, October 18, 2021 7:01:42 PM CEST Julia Lawall wrote: > > > > > > > > On Mon, 18 Oct 2021, Fabio M. De Francesco wrote: > > > > > > > > > On Monday, October 18, 2021 6:44:31 PM CEST Kushal Kothari wrote: > > > > > > Change the parameters of functions from const char *g_fbmode[] to > > > > > > const char * const g_fbmode[]. This additional const is needed to > > > > > > allow us to fix checkpatch warning, as well as being good > > > > > > programming practice. > > > > > > > > > > > > For the checkpatch warnings, if we have a set of command line > > > > > > args that we want to check defined as: > > > > > > static const char * g_fbmode[] = {NULL, NULL}; > > > > > > > > > > > > checkpatch will complain: > > > > > > WARNING: static const char * array should probably be static > > > const > > > > > char * const > > > > > > > > > > > > Signed-off-by: Kushal Kothari <kushalkothari285@gmail.com> > > > > > > --- > > > > > > > > > > > > [...] > > > > > > > > > > > > -static const char *g_fbmode[] = {NULL, NULL}; > > > > > > +static const char * const g_fbmode[] = {NULL, NULL}; > > > > > > > > > > You have introduced a logical change (g_fbmode[] entries cannot be > > > assigned > > > > > any more) and a build error (because there is code somewhere that > assigns > > > > > values to those slots). > > > > > > > > I wonder if this warning makes much sense when the array elements are > > > > NULL. I don't know if checkpatch could easily detect that. > > > > No, it couldn't really. It's a line by line parser and most frequently > > these are on separate lines. > > Sorry but I don't get it. For sure I'm missing something... > > I guess that checkpatch.pl warned Kushal because it detected that the array > of pointers was assigned with something and so it output that "static const > char * array should probably be static const". > > What I cannot understand is why it _can_ detect that the array is assigned > with some values but it _cannot_ check that those values are 'NULL' and so > avoid to warn. It's a simple rule. char *foo[] = { should probably be const char * const foo[] = { There are exceptions and that's why the message output is a warning and the text includes the word "probably" and not "must". It's up to the reader to determine if the warning is valid or not. That's true for _every_ output error, warning or strict message checkpatch emits. checkpatch is a stupid brainless script doing trivial pattern matches. People should _use_ their brains and not blindly trust and follow brainless scripts. And especially, everyone should compile and test changes _before_ submitting patches. It doesn't matter if a tool that produces a warning is checkpatch, sparse, coccinelle or a compiler. Validate and test _before_ submitting a patch. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Outreachy kernel] [PATCH] staging: sm750fb: make pointers in array const 2021-10-18 18:52 ` Joe Perches @ 2021-10-18 19:01 ` Fabio M. De Francesco 0 siblings, 0 replies; 13+ messages in thread From: Fabio M. De Francesco @ 2021-10-18 19:01 UTC (permalink / raw) To: Julia Lawall, Joe Perches Cc: sudipm.mukherjee, teddy.wang, gregkh, outreachy-kernel, mike.rapoport, kushalkothari2850, Kushal Kothari On Monday, October 18, 2021 8:52:41 PM CEST Joe Perches wrote: > On Mon, 2021-10-18 at 20:34 +0200, Fabio M. De Francesco wrote: > > On Monday, October 18, 2021 8:22:23 PM CEST Joe Perches wrote: > > > On Mon, 2021-10-18 at 19:18 +0200, Fabio M. De Francesco wrote: > > > > On Monday, October 18, 2021 7:01:42 PM CEST Julia Lawall wrote: > > > > > > > > > > On Mon, 18 Oct 2021, Fabio M. De Francesco wrote: > > > > > > > > > > > On Monday, October 18, 2021 6:44:31 PM CEST Kushal Kothari wrote: > > > > > > > Change the parameters of functions from const char *g_fbmode[] to > > > > > > > const char * const g_fbmode[]. This additional const is needed to > > > > > > > allow us to fix checkpatch warning, as well as being good > > > > > > > programming practice. > > > > > > > > > > > > > > For the checkpatch warnings, if we have a set of command line > > > > > > > args that we want to check defined as: > > > > > > > static const char * g_fbmode[] = {NULL, NULL}; > > > > > > > > > > > > > > checkpatch will complain: > > > > > > > WARNING: static const char * array should probably be static > > > > const > > > > > > char * const > > > > > > > > > > > > > > Signed-off-by: Kushal Kothari <kushalkothari285@gmail.com> > > > > > > > --- > > > > > > > > > > > > > > [...] > > > > > > > > > > > > > > -static const char *g_fbmode[] = {NULL, NULL}; > > > > > > > +static const char * const g_fbmode[] = {NULL, NULL}; > > > > > > > > > > > > You have introduced a logical change (g_fbmode[] entries cannot be > > > > assigned > > > > > > any more) and a build error (because there is code somewhere that > > assigns > > > > > > values to those slots). > > > > > > > > > > I wonder if this warning makes much sense when the array elements are > > > > > NULL. I don't know if checkpatch could easily detect that. > > > > > > No, it couldn't really. It's a line by line parser and most frequently > > > these are on separate lines. > > > > Sorry but I don't get it. For sure I'm missing something... > > > > I guess that checkpatch.pl warned Kushal because it detected that the array > > of pointers was assigned with something and so it output that "static const > > char * array should probably be static const". > > > > What I cannot understand is why it _can_ detect that the array is assigned > > with some values but it _cannot_ check that those values are 'NULL' and so > > avoid to warn. > > It's a simple rule. > > char *foo[] = { should probably be const char * const foo[] = { > > There are exceptions and that's why the message output is a warning > and the text includes the word "probably" and not "must". > > It's up to the reader to determine if the warning is valid or not. > > That's true for _every_ output error, warning or strict message > checkpatch emits. > > checkpatch is a stupid brainless script doing trivial pattern matches. > > People should _use_ their brains and not blindly trust and follow > brainless scripts. > > And especially, everyone should compile and test changes _before_ > submitting patches. > > It doesn't matter if a tool that produces a warning is checkpatch, > sparse, coccinelle or a compiler. > > Validate and test _before_ submitting a patch. > Yes, after some more thinking about this matter I agree with you :) This is why I wrote another message for explaining my new POV some minutes ago. Thanks, Fabio ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Outreachy kernel] [PATCH] staging: sm750fb: make pointers in array const 2021-10-18 16:54 ` [Outreachy kernel] " Fabio M. De Francesco 2021-10-18 17:01 ` Julia Lawall @ 2021-10-18 18:18 ` Fabio M. De Francesco 1 sibling, 0 replies; 13+ messages in thread From: Fabio M. De Francesco @ 2021-10-18 18:18 UTC (permalink / raw) To: sudipm.mukherjee, gregkh, outreachy-kernel, kushalkothari2850 Cc: teddy.wang, mike.rapoport, Kushal Kothari, Kushal Kothari, linux-staging, Karolina Drobnik, julia.lawall On Monday, October 18, 2021 6:54:34 PM CEST Fabio M. De Francesco wrote: > On Monday, October 18, 2021 6:44:31 PM CEST Kushal Kothari wrote: > > Change the parameters of functions from const char *g_fbmode[] to > > const char * const g_fbmode[]. This additional const is needed to > > allow us to fix checkpatch warning, as well as being good > > programming practice. > > > > For the checkpatch warnings, if we have a set of command line > > args that we want to check defined as: > > static const char * g_fbmode[] = {NULL, NULL}; > > > > checkpatch will complain: > > WARNING: static const char * array should probably be static const > char * const > > > > Signed-off-by: Kushal Kothari <kushalkothari285@gmail.com> > > --- > > drivers/staging/sm750fb/sm750.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/staging/sm750fb/sm750.c b/drivers/staging/sm750fb/ > sm750.c > > index dbd1159a2ef0..3d9b4b0efcb1 100644 > > --- a/drivers/staging/sm750fb/sm750.c > > +++ b/drivers/staging/sm750fb/sm750.c > > @@ -33,7 +33,7 @@ > > static int g_hwcursor = 1; > > static int g_noaccel; > > static int g_nomtrr; > > -static const char *g_fbmode[] = {NULL, NULL}; > > +static const char * const g_fbmode[] = {NULL, NULL}; > > You have introduced a logical change (g_fbmode[] entries cannot be assigned > any more) and a build error (because there is code somewhere that assigns > values to those slots). > > Please, build the code after you've changed it. > > Thanks, > > Fabio Hi Kushal, As I wrote your patch has errors, however, if it had no errors it could not be taken in any case. I just noticed that you forgot to CC linux- staging@lists.linux.dev. Some days ago, Greg Kroah-Hartman wrote clearly that he cannot take patches that are not submitted to linux-staging. The "Submit patches" section of https://kernelnewbies.org/Outreachyfirstpatch is a bit misleading. However, Karolina Drobnik is taking care to rework it. For the moment, please use the following command (or some smart variation of it) to get a list with _all_ the people and mailing lists you should send your patches to: kernel_source_dir@localhost:~ # perl scripts/get_maintainer.pl --separator , --norolestats -f drivers/staging/sm750fb/ The output is: Sudip Mukherjee <sudipm.mukherjee@gmail.com>,Teddy Wang <teddy.wang@siliconmotion.com>,Greg Kroah-Hartman <gregkh@linuxfoundation.org>,linux-fbdev@vger.kernel.org,linux- staging@lists.linux.dev,linux-kernel@vger.kernel.org Thanks, Fabio P.S.: @Karolina, please correct me if I wrote something that is wrong and that contradicts the new text you are working on. ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2021-10-18 19:45 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-10-18 16:44 [PATCH] staging: sm750fb: make pointers in array const Kushal Kothari 2021-10-18 16:54 ` [Outreachy kernel] " Fabio M. De Francesco 2021-10-18 17:01 ` Julia Lawall 2021-10-18 17:18 ` Fabio M. De Francesco 2021-10-18 18:22 ` Joe Perches 2021-10-18 18:34 ` Fabio M. De Francesco 2021-10-18 18:43 ` Julia Lawall 2021-10-18 18:56 ` kushal kothari 2021-10-18 19:53 ` Alison Schofield 2021-10-18 18:56 ` Fabio M. De Francesco 2021-10-18 18:52 ` Joe Perches 2021-10-18 19:01 ` Fabio M. De Francesco 2021-10-18 18:18 ` Fabio M. De Francesco
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.