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

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

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