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

[Alison Schofield <alison.schofield@intel.com>]

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.