Re: [PATCH] Staging: comedidev.h comedi_lrange should be const struct

[Arthur Brainville (Ybalrid)]

On Mon, Apr 10, 2017 at 10:01:05PM +0200, Greg KH wrote:
> On Mon, Apr 10, 2017 at 05:25:49PM +0200, Arthur Brainville wrote:
> > On Sun, Apr 09, 2017 at 09:02:03PM +0200, Greg KH wrote:
> > > On Sun, Apr 09, 2017 at 04:28:12PM +0200, Arthur Brainville (Ybalrid) wrote:
> > > > According to checkpatch.pl, comedi_lrange should be declared as `const
> > > > struct` instead of `struct` in driver/staging/comedidev.h
> > > >
> > > > Signed-off-by: Arthur Brainville (Ybalrid) <ybalrid@xxxxxxxxxxxx>
> > > > ---
> > > >  drivers/staging/comedi/comedidev.h | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/staging/comedi/comedidev.h b/drivers/staging/comedi/comedidev.h
> > > > index 1bb9986f865e..82df090783b5 100644
> > > > --- a/drivers/staging/comedi/comedidev.h
> > > > +++ b/drivers/staging/comedi/comedidev.h
> > > > @@ -623,7 +623,7 @@ extern const struct comedi_lrange range_unknown;
> > > >   * There may also be a flag that indicates the minimum and maximum are merely
> > > >   * scale factors for an unknown, external reference.
> > > >   */
> > > > -struct comedi_lrange {
> > > > +const struct comedi_lrange {
> > > >  	int length;
> > > >  	struct comedi_krange range[];
> > > >  };
> > > 
> > > Huh?  Please explain how this change is correct.
> > > 
> > > greg k-h
> > 
> > First of all, thanks for taking the time to reply!
> > Well, sorry if I did something wrong, I'm a newbie here, so let me
> > explain:
> > 
> > When checking a bunch of files with /script/checkpatch.pl -f on the
> > staging directory, it gave me this output:
> > 
> > > WARNING: struct comedi_lrange should normally be const
> > > #626: FILE: drivers/staging/comedi/comedidev.h:626:
> > > +struct comedi_lrange {
> > > 
> > > total: 0 errors, 1 warnings, 6 checks, 1043 lines checked
> > > 
> > > NOTE: For some of the reported defects, checkpatch may be able to
> > >      mechanically convert to the typical style using --fix or
> > >      --fix-inplace.
> > 
> > For this specific header.
> > 
> > Declaring it as const struct fixes that coding style warning, and
> > doesn't break the build but, AFAIK isn't actually meaningfull since it's
> > declaring a struct, but not any variable of type "comedi_lrange"
> > at this point. And it built fine.
> > I was watching an old talk (from you) about how you can send a patch to
> > correct this kind of little things, so I tried.
> > 
> > Looking further into it, it seems that this change makes GCC trigger the
> > following warning in any file that #include it :
> > > In file included from drivers/staging/comedi//comedi_usb.h:24:0,
> > >                 from drivers/staging/comedi//comedi_usb.c:21:
> > >                 drivers/staging/comedi//comedidev.h:629:1: warning:
> > >                 useless type qualifier in empty declaration
> > >                  };
> > 
> > So, probably not good?
> 
> You should always test-build your patches, and they can never add new
> kernel warnings to the build, that's not allowed at all.  So please do
> that next time, it should have shown you that this was not a good
> change.
> 
> And as you have found out, checkpatch.pl is a dumb tool, you still have
> to think when you use it.  And if you know C, you will know that the
> change you made makes no sense at all :)
> 
> thanks,
> 
> greg k-h

Lesson learned!

(It did build. But well, I don't know what I was thinking not paying
attentions to theses warning. Shame on me!)

Thank you again for your patience Greg :)

Arthur Brainville