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

["Fabio M. De Francesco" <fmdefrancesco@gmail.com>]

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