[PATCH] staging: sm750fb: make pointers in array const

[PATCH] staging: sm750fb: make pointers in array const

[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

Re: [Outreachy kernel] [PATCH] staging: sm750fb: make pointers in array const

[Fabio M. De Francesco]

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.
> 

Re: [Outreachy kernel] [PATCH] staging: sm750fb: make pointers in array const

[Julia Lawall]

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.
>

Re: [Outreachy kernel] [PATCH] staging: sm750fb: make pointers in array const

[Fabio M. De Francesco]

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
> > >

Re: [Outreachy kernel] [PATCH] staging: sm750fb: make pointers in array const

[Fabio M. De Francesco]

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.

Re: [Outreachy kernel] [PATCH] staging: sm750fb: make pointers in array const

[Joe Perches]

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.

Re: [Outreachy kernel] [PATCH] staging: sm750fb: make pointers in array const

[Fabio M. De Francesco]

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.
> 
> 
> 

Re: [Outreachy kernel] [PATCH] staging: sm750fb: make pointers in array const

[Julia Lawall]

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.
> >
> >
> >
>
>
>
>
>

Re: [Outreachy kernel] [PATCH] staging: sm750fb: make pointers in array const

[Joe Perches]

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.

Re: [Outreachy kernel] [PATCH] staging: sm750fb: make pointers in array const

[kushal kothari]

[-- 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 --]

Re: [Outreachy kernel] [PATCH] staging: sm750fb: make pointers in array const

[Fabio M. De Francesco]

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.
> > >
> > >
> > >
> >
> >
> >
> >
> >
> 

Re: [Outreachy kernel] [PATCH] staging: sm750fb: make pointers in array const

[Fabio M. De Francesco]

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

Re: [Outreachy kernel] [PATCH] staging: sm750fb: make pointers in array const

[Alison Schofield]

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.