* checkpatch complaint @ 2012-02-08 20:51 Arend van Spriel 2012-02-08 23:42 ` Joe Perches 0 siblings, 1 reply; 5+ messages in thread From: Arend van Spriel @ 2012-02-08 20:51 UTC (permalink / raw) To: Andy Whitcroft; +Cc: Dan Carpenter, Joe Perches, linux-kernel checkpatch complains on code below and I must overlook something or checkpatch gives a false negative/positive/whatever: #define IFPTR(usb, idx) ((usb)->actconfig->interface[(idx)]) #define IFALTS(usb, idx) (IFPTR((usb), (idx))->altsetting[0]) #define IFDESC(usb, idx) IFALTS((usb), (idx)).desc #define IFEPDESC(usb, idx, ep) \ (IFALTS((usb), (idx)).endpoint[(ep)]).desc checkpatch errors: ERROR: Macros with complex values should be enclosed in parenthesis #169: FILE: drivers/net/wireless/brcm80211/brcmfmac/usb.c:58: +#define IFDESC(usb, idx) (IFALTS((usb), (idx))).desc ERROR: Macros with complex values should be enclosed in parenthesis #170: FILE: drivers/net/wireless/brcm80211/brcmfmac/usb.c:59: +#define IFEPDESC(usb, idx, ep) ((IFALTS((usb), (idx))).endpoint[(ep)]).desc Any ideas? I tried extra parenthesis around IFALTS but that does not resolve it. Gr. AvS ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: checkpatch complaint 2012-02-08 20:51 checkpatch complaint Arend van Spriel @ 2012-02-08 23:42 ` Joe Perches 2012-02-09 5:05 ` Guenter Roeck 2012-02-09 10:06 ` Dan Carpenter 0 siblings, 2 replies; 5+ messages in thread From: Joe Perches @ 2012-02-08 23:42 UTC (permalink / raw) To: Arend van Spriel; +Cc: Andy Whitcroft, Dan Carpenter, linux-kernel On Wed, 2012-02-08 at 21:51 +0100, Arend van Spriel wrote: > checkpatch complains on code below and I must overlook something or > checkpatch gives a false negative/positive/whatever: > > #define IFPTR(usb, idx) ((usb)->actconfig->interface[(idx)]) > #define IFALTS(usb, idx) (IFPTR((usb), (idx))->altsetting[0]) > #define IFDESC(usb, idx) IFALTS((usb), (idx)).desc > #define IFEPDESC(usb, idx, ep) \ > (IFALTS((usb), (idx)).endpoint[(ep)]).desc > > checkpatch errors: > ERROR: Macros with complex values should be enclosed in parenthesis > #169: FILE: drivers/net/wireless/brcm80211/brcmfmac/usb.c:58: > +#define IFDESC(usb, idx) (IFALTS((usb), (idx))).desc > > ERROR: Macros with complex values should be enclosed in parenthesis > #170: FILE: drivers/net/wireless/brcm80211/brcmfmac/usb.c:59: > +#define IFEPDESC(usb, idx, ep) ((IFALTS((usb), > (idx))).endpoint[(ep)]).desc > > Any ideas? I tried extra parenthesis around IFALTS but that does not > resolve it. I think the entries should be surround by () I think it's reasonable too. I tried: $ cat cp.c #define IFPTR(usb, idx) ((usb)->actconfig->interface[(idx)]) #define IFALTS(usb, idx) (IFPTR((usb), (idx))->altsetting[0]) #define IFDESC(usb, idx) (IFALTS((usb), (idx)).desc) #define IFEPDESC(usb, idx, ep) ((IFALTS((usb), (idx)).endpoint[(ep)]).desc) $ ./scripts/checkpatch.pl -f cp.c total: 0 errors, 0 warnings, 5 lines checked cp.c has no obvious style problems and is ready for submission. ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: checkpatch complaint 2012-02-08 23:42 ` Joe Perches @ 2012-02-09 5:05 ` Guenter Roeck 2012-02-09 10:20 ` Bernd Petrovitsch 2012-02-09 10:06 ` Dan Carpenter 1 sibling, 1 reply; 5+ messages in thread From: Guenter Roeck @ 2012-02-09 5:05 UTC (permalink / raw) To: Joe Perches; +Cc: Arend van Spriel, Andy Whitcroft, Dan Carpenter, linux-kernel On Wed, Feb 08, 2012 at 06:42:14PM -0500, Joe Perches wrote: > On Wed, 2012-02-08 at 21:51 +0100, Arend van Spriel wrote: > > checkpatch complains on code below and I must overlook something or > > checkpatch gives a false negative/positive/whatever: > > > > #define IFPTR(usb, idx) ((usb)->actconfig->interface[(idx)]) > > #define IFALTS(usb, idx) (IFPTR((usb), (idx))->altsetting[0]) > > #define IFDESC(usb, idx) IFALTS((usb), (idx)).desc > > #define IFEPDESC(usb, idx, ep) \ > > (IFALTS((usb), (idx)).endpoint[(ep)]).desc > > > > checkpatch errors: > > ERROR: Macros with complex values should be enclosed in parenthesis > > #169: FILE: drivers/net/wireless/brcm80211/brcmfmac/usb.c:58: > > +#define IFDESC(usb, idx) (IFALTS((usb), (idx))).desc > > > > ERROR: Macros with complex values should be enclosed in parenthesis > > #170: FILE: drivers/net/wireless/brcm80211/brcmfmac/usb.c:59: > > +#define IFEPDESC(usb, idx, ep) ((IFALTS((usb), > > (idx))).endpoint[(ep)]).desc > > > > Any ideas? I tried extra parenthesis around IFALTS but that does not > > resolve it. > > I think the entries should be surround by () > I think it's reasonable too. > > I tried: > > $ cat cp.c > #define IFPTR(usb, idx) ((usb)->actconfig->interface[(idx)]) > #define IFALTS(usb, idx) (IFPTR((usb), (idx))->altsetting[0]) > #define IFDESC(usb, idx) (IFALTS((usb), (idx)).desc) > #define IFEPDESC(usb, idx, ep) ((IFALTS((usb), (idx)).endpoint[(ep)]).desc) > > $ ./scripts/checkpatch.pl -f cp.c > total: 0 errors, 0 warnings, 5 lines checked > > cp.c has no obvious style problems and is ready for submission. > Hi Joe, different example, same message: ERROR: Macros with complex values should be enclosed in parenthesis #339: FILE: hwmon/ltc4245.c:339: +#define LTC4245_ALARM(name, mask, reg) \ + static SENSOR_DEVICE_ATTR_2(name, S_IRUGO, \ + ltc4245_show_alarm, NULL, (mask), reg) and: ERROR: Macros with complex values should be enclosed in parenthesis #585: FILE: hwmon/pc87360.c:585: +#define VIN_UNIT_ATTRS(X) \ + &in_input[X].dev_attr.attr, \ + &in_status[X].dev_attr.attr, \ + &in_min[X].dev_attr.attr, \ + &in_max[X].dev_attr.attr, \ + &in_min_alarm[X].dev_attr.attr, \ + &in_max_alarm[X].dev_attr.attr and: ERROR: Macros with multiple statements should be enclosed in a do - while loop #291: FILE: hwmon/sis5595.c:291: +#define show_in_offset(offset) \ +static SENSOR_DEVICE_ATTR(in##offset##_input, S_IRUGO, \ + show_in, NULL, offset); \ +static SENSOR_DEVICE_ATTR(in##offset##_min, S_IRUGO | S_IWUSR, \ + show_in_min, set_in_min, offset); \ +static SENSOR_DEVICE_ATTR(in##offset##_max, S_IRUGO | S_IWUSR, \ + show_in_max, set_in_max, offset); Those are all pretty common in the hwmon subsystem and all quite annnoying. Other than labeling them as "false positive", I have no idea what to do about it. Sure, we could replace all the macros with immediate code, but that doesn't sound very compelling. If you have an idea, please let me know. Thanks, Guenter ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: checkpatch complaint 2012-02-09 5:05 ` Guenter Roeck @ 2012-02-09 10:20 ` Bernd Petrovitsch 0 siblings, 0 replies; 5+ messages in thread From: Bernd Petrovitsch @ 2012-02-09 10:20 UTC (permalink / raw) To: Guenter Roeck Cc: Joe Perches, Arend van Spriel, Andy Whitcroft, Dan Carpenter, linux-kernel On Mit, 2012-02-08 at 21:05 -0800, Guenter Roeck wrote: > On Wed, Feb 08, 2012 at 06:42:14PM -0500, Joe Perches wrote: > > On Wed, 2012-02-08 at 21:51 +0100, Arend van Spriel wrote: > > > checkpatch complains on code below and I must overlook something or > > > checkpatch gives a false negative/positive/whatever: > > > > > > #define IFPTR(usb, idx) ((usb)->actconfig->interface[(idx)]) > > > #define IFALTS(usb, idx) (IFPTR((usb), (idx))->altsetting[0]) > > > #define IFDESC(usb, idx) IFALTS((usb), (idx)).desc > > > #define IFEPDESC(usb, idx, ep) \ > > > (IFALTS((usb), (idx)).endpoint[(ep)]).desc > > > > > > checkpatch errors: > > > ERROR: Macros with complex values should be enclosed in parenthesis > > > #169: FILE: drivers/net/wireless/brcm80211/brcmfmac/usb.c:58: > > > +#define IFDESC(usb, idx) (IFALTS((usb), (idx))).desc > > > > > > ERROR: Macros with complex values should be enclosed in parenthesis > > > #170: FILE: drivers/net/wireless/brcm80211/brcmfmac/usb.c:59: > > > +#define IFEPDESC(usb, idx, ep) ((IFALTS((usb), > > > (idx))).endpoint[(ep)]).desc > > > > > > Any ideas? I tried extra parenthesis around IFALTS but that does not > > > resolve it. You probably did it wrong. Sent the next time what you *really* did and not only description ..... > > I think the entries should be surround by () > > I think it's reasonable too. > > > > I tried: > > > > $ cat cp.c > > #define IFPTR(usb, idx) ((usb)->actconfig->interface[(idx)]) > > #define IFALTS(usb, idx) (IFPTR((usb), (idx))->altsetting[0]) > > #define IFDESC(usb, idx) (IFALTS((usb), (idx)).desc) > > #define IFEPDESC(usb, idx, ep) ((IFALTS((usb), (idx)).endpoint[(ep)]).desc) > > > > $ ./scripts/checkpatch.pl -f cp.c > > total: 0 errors, 0 warnings, 5 lines checked > > > > cp.c has no obvious style problems and is ready for submission. > > > Hi Joe, > > different example, same message: > > ERROR: Macros with complex values should be enclosed in parenthesis > #339: FILE: hwmon/ltc4245.c:339: > +#define LTC4245_ALARM(name, mask, reg) \ > + static SENSOR_DEVICE_ATTR_2(name, S_IRUGO, \ > + ltc4245_show_alarm, NULL, (mask), reg) *eg* Make it one (long) line. IMHO this is a prototype example to ignore the 80-column limit. I haven't looked the the reg-exps (or the code in `checkpatch`) but either make `checkpatch` recognize the "\" at the end of lines and merge the lines before doing these checks (since they apparently assume that the complete macro definition is on one line) or allow "\\\n" within the reg-exps. > and: > > ERROR: Macros with complex values should be enclosed in parenthesis > #585: FILE: hwmon/pc87360.c:585: > +#define VIN_UNIT_ATTRS(X) \ > + &in_input[X].dev_attr.attr, \ > + &in_status[X].dev_attr.attr, \ > + &in_min[X].dev_attr.attr, \ > + &in_max[X].dev_attr.attr, \ > + &in_min_alarm[X].dev_attr.attr, \ > + &in_max_alarm[X].dev_attr.attr [...] > Other than labeling them as "false positive", I have no idea what to do about it. > Sure, we could replace all the macros with immediate code, but that doesn't sound > very compelling. If you have an idea, please let me know. Given that the above gives after the preprocessor the desired results: Add special comments (like `indent` allows to disable it temporarily) so that checkpatch doesn't complain about these macros. Yes, that can be abused but review should catch that. And newbies won't see false positives. Listing the to-be-ignored #define's in `checkpatch` (or some external file) or enhancing it with some "known false positives, do not emit *that* warning there" is also a maintenance burden (hmm, how often do they change?). Bernd -- Bernd Petrovitsch Email : bernd@petrovitsch.priv.at LUGA : http://www.luga.at ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: checkpatch complaint 2012-02-08 23:42 ` Joe Perches 2012-02-09 5:05 ` Guenter Roeck @ 2012-02-09 10:06 ` Dan Carpenter 1 sibling, 0 replies; 5+ messages in thread From: Dan Carpenter @ 2012-02-09 10:06 UTC (permalink / raw) To: Joe Perches; +Cc: Arend van Spriel, Andy Whitcroft, linux-kernel [-- Attachment #1: Type: text/plain, Size: 1792 bytes --] On Wed, Feb 08, 2012 at 03:42:14PM -0800, Joe Perches wrote: > On Wed, 2012-02-08 at 21:51 +0100, Arend van Spriel wrote: > > checkpatch complains on code below and I must overlook something or > > checkpatch gives a false negative/positive/whatever: > > > > #define IFPTR(usb, idx) ((usb)->actconfig->interface[(idx)]) > > #define IFALTS(usb, idx) (IFPTR((usb), (idx))->altsetting[0]) > > #define IFDESC(usb, idx) IFALTS((usb), (idx)).desc > > #define IFEPDESC(usb, idx, ep) \ > > (IFALTS((usb), (idx)).endpoint[(ep)]).desc > > > > checkpatch errors: > > ERROR: Macros with complex values should be enclosed in parenthesis > > #169: FILE: drivers/net/wireless/brcm80211/brcmfmac/usb.c:58: > > +#define IFDESC(usb, idx) (IFALTS((usb), (idx))).desc > > > > ERROR: Macros with complex values should be enclosed in parenthesis > > #170: FILE: drivers/net/wireless/brcm80211/brcmfmac/usb.c:59: > > +#define IFEPDESC(usb, idx, ep) ((IFALTS((usb), > > (idx))).endpoint[(ep)]).desc > > > > Any ideas? I tried extra parenthesis around IFALTS but that does not > > resolve it. > > I think the entries should be surround by () > I think it's reasonable too. > No. That's not reasonable. The other code is: A) perfectly fine B) nicer to look at We already have newbies running checkpatch.pl -f against the kernel source and sending bogus patches that make the code uglier. I've tried to fight back against checkpatch patches before where they make the code worse, but it's just overwhelming. I don't like to be the bad guy to tell newbies that they are sending bad patches when actually the Official Kernel Checkpatch tool said they should send it. We're causing everyone pain for no reason. regards, dan carpenter [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 836 bytes --] ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2012-02-09 10:21 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2012-02-08 20:51 checkpatch complaint Arend van Spriel 2012-02-08 23:42 ` Joe Perches 2012-02-09 5:05 ` Guenter Roeck 2012-02-09 10:20 ` Bernd Petrovitsch 2012-02-09 10:06 ` Dan Carpenter
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).