* Re: [PATCH] Staging: comedidev.h comedi_lrange should be const struct [not found] <20170409194120.GA23199@Leliel> @ 2017-04-10 15:25 ` Arthur Brainville 2017-04-10 20:01 ` Greg KH 0 siblings, 1 reply; 7+ messages in thread From: Arthur Brainville @ 2017-04-10 15:25 UTC (permalink / raw) To: Greg KH; +Cc: devel, linux-kernel, ybalrid 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@ybalrid.info> > > --- > > 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? Like I said here the `const` qualifier doen't change anything, you could create a `struct comedi_lrange` variable with some initialization, and change it later, it will not be const, unless declared so at that point. It was just to make this file pass the checkpatch perl script without warning, but it makes the staging comedi driver compile with warnings. It doesn't change anything "functionally", if you see what I mean. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] Staging: comedidev.h comedi_lrange should be const struct 2017-04-10 15:25 ` [PATCH] Staging: comedidev.h comedi_lrange should be const struct Arthur Brainville @ 2017-04-10 20:01 ` Greg KH 2017-04-10 20:17 ` Arthur Brainville (Ybalrid) 0 siblings, 1 reply; 7+ messages in thread From: Greg KH @ 2017-04-10 20:01 UTC (permalink / raw) To: Arthur Brainville; +Cc: devel, linux-kernel 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@ybalrid.info> > > > --- > > > 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] Staging: comedidev.h comedi_lrange should be const struct 2017-04-10 20:01 ` Greg KH @ 2017-04-10 20:17 ` Arthur Brainville (Ybalrid) 0 siblings, 0 replies; 7+ messages in thread From: Arthur Brainville (Ybalrid) @ 2017-04-10 20:17 UTC (permalink / raw) To: Greg KH; +Cc: devel, linux-kernel 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@ybalrid.info> > > > > --- > > > > 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH] Staging: comedidev.h comedi_lrange should be const struct @ 2017-04-09 14:28 Arthur Brainville (Ybalrid) 2017-04-09 16:22 ` kbuild test robot ` (2 more replies) 0 siblings, 3 replies; 7+ messages in thread From: Arthur Brainville (Ybalrid) @ 2017-04-09 14:28 UTC (permalink / raw) To: abbotti; +Cc: devel, linux-kernel, Arthur Brainville (Ybalrid) 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@ybalrid.info> --- 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[]; }; -- 2.12.2 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] Staging: comedidev.h comedi_lrange should be const struct 2017-04-09 14:28 Arthur Brainville (Ybalrid) @ 2017-04-09 16:22 ` kbuild test robot 2017-04-09 19:02 ` Greg KH 2017-04-10 2:51 ` kbuild test robot 2 siblings, 0 replies; 7+ messages in thread From: kbuild test robot @ 2017-04-09 16:22 UTC (permalink / raw) To: Arthur Brainville (Ybalrid) Cc: kbuild-all, abbotti, devel, linux-kernel, Arthur Brainville (Ybalrid) [-- Attachment #1: Type: text/plain, Size: 3473 bytes --] Hi Arthur, [auto build test WARNING on staging/staging-testing] [also build test WARNING on v4.11-rc5 next-20170407] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] url: https://github.com/0day-ci/linux/commits/Arthur-Brainville-Ybalrid/Staging-comedidev-h-comedi_lrange-should-be-const-struct/20170409-224503 config: x86_64-allyesdebian (attached as .config) compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901 reproduce: # save the attached .config to linux build tree make ARCH=x86_64 All warnings (new ones prefixed by >>): In file included from drivers/staging/comedi/comedi_fops.c:34:0: >> drivers/staging/comedi/comedidev.h:629:1: warning: useless type qualifier in empty declaration }; ^ vim +629 drivers/staging/comedi/comedidev.h ed9eccbe8 David Schleef 2008-11-04 613 #define range_digital range_unipolar5 ed9eccbe8 David Schleef 2008-11-04 614 5459bc128 Ian Abbott 2015-09-21 615 /** 5459bc128 Ian Abbott 2015-09-21 616 * struct comedi_lrange - Describes a COMEDI range table 5459bc128 Ian Abbott 2015-09-21 617 * @length: Number of entries in the range table. 5459bc128 Ian Abbott 2015-09-21 618 * @range: Array of &struct comedi_krange, one for each range. 5459bc128 Ian Abbott 2015-09-21 619 * 5459bc128 Ian Abbott 2015-09-21 620 * Each element of @range[] describes the minimum and maximum physical range 5459bc128 Ian Abbott 2015-09-21 621 * range and the type of units. Typically, the type of unit is %UNIT_volt 5459bc128 Ian Abbott 2015-09-21 622 * (i.e. volts) and the minimum and maximum are in millionths of a volt. 5459bc128 Ian Abbott 2015-09-21 623 * There may also be a flag that indicates the minimum and maximum are merely 5459bc128 Ian Abbott 2015-09-21 624 * scale factors for an unknown, external reference. 5459bc128 Ian Abbott 2015-09-21 625 */ 540e454c2 Arthur Brainville (Ybalrid 2017-04-09 626) const struct comedi_lrange { ed9eccbe8 David Schleef 2008-11-04 627 int length; 3358a0ca2 Cheah Kok Cheong 2016-12-22 628 struct comedi_krange range[]; ed9eccbe8 David Schleef 2008-11-04 @629 }; ed9eccbe8 David Schleef 2008-11-04 630 42e558399 Ian Abbott 2015-09-21 631 /** 42e558399 Ian Abbott 2015-09-21 632 * comedi_range_is_bipolar() - Test if subdevice range is bipolar 42e558399 Ian Abbott 2015-09-21 633 * @s: COMEDI subdevice. 42e558399 Ian Abbott 2015-09-21 634 * @range: Index of range within a range table. 42e558399 Ian Abbott 2015-09-21 635 * 42e558399 Ian Abbott 2015-09-21 636 * Tests whether a range is bipolar by checking whether its minimum value 42e558399 Ian Abbott 2015-09-21 637 * is negative. :::::: The code at line 629 was first introduced by commit :::::: ed9eccbe8970f6eedc1b978c157caf1251a896d4 Staging: add comedi core :::::: TO: David Schleef <ds@schleef.org> :::::: CC: Greg Kroah-Hartman <gregkh@suse.de> --- 0-DAY kernel test infrastructure Open Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation [-- Attachment #2: .config.gz --] [-- Type: application/gzip, Size: 38598 bytes --] ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] Staging: comedidev.h comedi_lrange should be const struct 2017-04-09 14:28 Arthur Brainville (Ybalrid) 2017-04-09 16:22 ` kbuild test robot @ 2017-04-09 19:02 ` Greg KH 2017-04-10 2:51 ` kbuild test robot 2 siblings, 0 replies; 7+ messages in thread From: Greg KH @ 2017-04-09 19:02 UTC (permalink / raw) To: Arthur Brainville (Ybalrid); +Cc: abbotti, devel, linux-kernel 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@ybalrid.info> > --- > 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] Staging: comedidev.h comedi_lrange should be const struct 2017-04-09 14:28 Arthur Brainville (Ybalrid) 2017-04-09 16:22 ` kbuild test robot 2017-04-09 19:02 ` Greg KH @ 2017-04-10 2:51 ` kbuild test robot 2 siblings, 0 replies; 7+ messages in thread From: kbuild test robot @ 2017-04-10 2:51 UTC (permalink / raw) To: Arthur Brainville (Ybalrid) Cc: kbuild-all, abbotti, devel, linux-kernel, Arthur Brainville (Ybalrid) Hi Arthur, [auto build test WARNING on staging/staging-testing] [also build test WARNING on v4.11-rc6 next-20170407] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] url: https://github.com/0day-ci/linux/commits/Arthur-Brainville-Ybalrid/Staging-comedidev-h-comedi_lrange-should-be-const-struct/20170409-224503 config: x86_64-allmodconfig compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901 reproduce: make ARCH=x86_64 allmodconfig make ARCH=x86_64 All warnings (new ones prefixed by >>): vim +629 drivers/staging/comedi/drivers/../comedidev.h ed9eccbe8 David Schleef 2008-11-04 613 #define range_digital range_unipolar5 ed9eccbe8 David Schleef 2008-11-04 614 5459bc128 Ian Abbott 2015-09-21 615 /** 5459bc128 Ian Abbott 2015-09-21 616 * struct comedi_lrange - Describes a COMEDI range table 5459bc128 Ian Abbott 2015-09-21 617 * @length: Number of entries in the range table. 5459bc128 Ian Abbott 2015-09-21 618 * @range: Array of &struct comedi_krange, one for each range. 5459bc128 Ian Abbott 2015-09-21 619 * 5459bc128 Ian Abbott 2015-09-21 620 * Each element of @range[] describes the minimum and maximum physical range 5459bc128 Ian Abbott 2015-09-21 621 * range and the type of units. Typically, the type of unit is %UNIT_volt 5459bc128 Ian Abbott 2015-09-21 622 * (i.e. volts) and the minimum and maximum are in millionths of a volt. 5459bc128 Ian Abbott 2015-09-21 623 * There may also be a flag that indicates the minimum and maximum are merely 5459bc128 Ian Abbott 2015-09-21 624 * scale factors for an unknown, external reference. 5459bc128 Ian Abbott 2015-09-21 625 */ 540e454c2 Arthur Brainville (Ybalrid 2017-04-09 626) const struct comedi_lrange { ed9eccbe8 David Schleef 2008-11-04 627 int length; 3358a0ca2 Cheah Kok Cheong 2016-12-22 628 struct comedi_krange range[]; ed9eccbe8 David Schleef 2008-11-04 @629 }; ed9eccbe8 David Schleef 2008-11-04 630 42e558399 Ian Abbott 2015-09-21 631 /** 42e558399 Ian Abbott 2015-09-21 632 * comedi_range_is_bipolar() - Test if subdevice range is bipolar 42e558399 Ian Abbott 2015-09-21 633 * @s: COMEDI subdevice. 42e558399 Ian Abbott 2015-09-21 634 * @range: Index of range within a range table. 42e558399 Ian Abbott 2015-09-21 635 * 42e558399 Ian Abbott 2015-09-21 636 * Tests whether a range is bipolar by checking whether its minimum value 42e558399 Ian Abbott 2015-09-21 637 * is negative. :::::: The code at line 629 was first introduced by commit :::::: ed9eccbe8970f6eedc1b978c157caf1251a896d4 Staging: add comedi core :::::: TO: David Schleef <ds@schleef.org> :::::: CC: Greg Kroah-Hartman <gregkh@suse.de> --- 0-DAY kernel test infrastructure Open Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2017-04-10 20:17 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <20170409194120.GA23199@Leliel> 2017-04-10 15:25 ` [PATCH] Staging: comedidev.h comedi_lrange should be const struct Arthur Brainville 2017-04-10 20:01 ` Greg KH 2017-04-10 20:17 ` Arthur Brainville (Ybalrid) 2017-04-09 14:28 Arthur Brainville (Ybalrid) 2017-04-09 16:22 ` kbuild test robot 2017-04-09 19:02 ` Greg KH 2017-04-10 2:51 ` kbuild test robot
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.