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

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

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