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
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
