Re: [Ksummit-2013-discuss] [PATCH] checkpatch: Add comment about updating Documentation/CodingStyle
On Tue, Sep 03, 2013 at 12:09:31PM -0600, Bjorn Helgaas wrote: > On Mon, Sep 2, 2013 at 6:39 PM, Fengguang Wu wrote: > > > Thanks! Yes I'm now running checkpatch these days because some people > > suggested to me that some of the checkpatch warnings do help catch > > real bugs. > > > > However I do try to avoid upsetting people with maybe-subjective > > warnings. A checkpatch report will only be sent when a small fraction > > of error types are detected. Comments are very welcome on how to > > improve this list: > > How hard would it be to make this configurable per-git tree? It'd be trivial to do. > I think the robot is quite useful and I don't push branches very > often, so I'd probably be OK with just getting all the checkpatch > warnings along with the build warnings and filtering the noise > myself. But I know other people have different styles and opinions. Would you tell me the git tree/branches that would like to receive all checkpatch warnings? It'll should be useful for our internal kernel team, too. Thanks, Fengguang > > MEMSET > > IN_ATOMIC > > UAPI_INCLUDE > > MALFORMED_INCLUDE > > SIZEOF_ADDRESS > > KREALLOC_ARG_REUSE > > EXECUTE_PERMISSIONS > > ERROR:BAD_SIGN_OFF > > LO_MACRO > > HI_MACRO > > CSYNC > > SSYNC > > HOTPLUG_SECTION > > INDENTED_LABEL > > INLINE_LOCATION > > STORAGE_CLASS > > USLEEP_RANGE > > UNNECESSARY_CASTS > > ALLOC_SIZEOF_STRUCT > > KREALLOC_ARG_REUSE > > USE_FUNC > > LOCKDEP > > EXPORTED_WORLD_WRITABLE > > WHITESPACE_AFTER_LINE_CONTINUATION > > MISSING_VMLINUX_SYMBOL > > NEEDLESS_IF > > PRINTF_L > > > > Once the decision is made to send a checkpatch error/warning, the > > report email will use the triggering error (the one that matters) as > > the email subject, with the complete output of checkpatch.pl included > > in email body. > > > > Thanks, > > Fengguang > > ___ > > Ksummit-2013-discuss mailing list > > ksummit-2013-disc...@lists.linuxfoundation.org > > https://lists.linuxfoundation.org/mailman/listinfo/ksummit-2013-discuss -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Ksummit-2013-discuss] [PATCH] checkpatch: Add comment about updating Documentation/CodingStyle
On Mon, Sep 2, 2013 at 6:39 PM, Fengguang Wu wrote: > Thanks! Yes I'm now running checkpatch these days because some people > suggested to me that some of the checkpatch warnings do help catch > real bugs. > > However I do try to avoid upsetting people with maybe-subjective > warnings. A checkpatch report will only be sent when a small fraction > of error types are detected. Comments are very welcome on how to > improve this list: How hard would it be to make this configurable per-git tree? I think the robot is quite useful and I don't push branches very often, so I'd probably be OK with just getting all the checkpatch warnings along with the build warnings and filtering the noise myself. But I know other people have different styles and opinions. > MEMSET > IN_ATOMIC > UAPI_INCLUDE > MALFORMED_INCLUDE > SIZEOF_ADDRESS > KREALLOC_ARG_REUSE > EXECUTE_PERMISSIONS > ERROR:BAD_SIGN_OFF > LO_MACRO > HI_MACRO > CSYNC > SSYNC > HOTPLUG_SECTION > INDENTED_LABEL > INLINE_LOCATION > STORAGE_CLASS > USLEEP_RANGE > UNNECESSARY_CASTS > ALLOC_SIZEOF_STRUCT > KREALLOC_ARG_REUSE > USE_FUNC > LOCKDEP > EXPORTED_WORLD_WRITABLE > WHITESPACE_AFTER_LINE_CONTINUATION > MISSING_VMLINUX_SYMBOL > NEEDLESS_IF > PRINTF_L > > Once the decision is made to send a checkpatch error/warning, the > report email will use the triggering error (the one that matters) as > the email subject, with the complete output of checkpatch.pl included > in email body. > > Thanks, > Fengguang > ___ > Ksummit-2013-discuss mailing list > ksummit-2013-disc...@lists.linuxfoundation.org > https://lists.linuxfoundation.org/mailman/listinfo/ksummit-2013-discuss -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Ksummit-2013-discuss] [PATCH] checkpatch: Add comment about updating Documentation/CodingStyle
On Mon, Sep 2, 2013 at 6:39 PM, Fengguang Wu fengguang...@intel.com wrote: Thanks! Yes I'm now running checkpatch these days because some people suggested to me that some of the checkpatch warnings do help catch real bugs. However I do try to avoid upsetting people with maybe-subjective warnings. A checkpatch report will only be sent when a small fraction of error types are detected. Comments are very welcome on how to improve this list: How hard would it be to make this configurable per-git tree? I think the robot is quite useful and I don't push branches very often, so I'd probably be OK with just getting all the checkpatch warnings along with the build warnings and filtering the noise myself. But I know other people have different styles and opinions. MEMSET IN_ATOMIC UAPI_INCLUDE MALFORMED_INCLUDE SIZEOF_ADDRESS KREALLOC_ARG_REUSE EXECUTE_PERMISSIONS ERROR:BAD_SIGN_OFF LO_MACRO HI_MACRO CSYNC SSYNC HOTPLUG_SECTION INDENTED_LABEL INLINE_LOCATION STORAGE_CLASS USLEEP_RANGE UNNECESSARY_CASTS ALLOC_SIZEOF_STRUCT KREALLOC_ARG_REUSE USE_FUNC LOCKDEP EXPORTED_WORLD_WRITABLE WHITESPACE_AFTER_LINE_CONTINUATION MISSING_VMLINUX_SYMBOL NEEDLESS_IF PRINTF_L Once the decision is made to send a checkpatch error/warning, the report email will use the triggering error (the one that matters) as the email subject, with the complete output of checkpatch.pl included in email body. Thanks, Fengguang ___ Ksummit-2013-discuss mailing list ksummit-2013-disc...@lists.linuxfoundation.org https://lists.linuxfoundation.org/mailman/listinfo/ksummit-2013-discuss -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Ksummit-2013-discuss] [PATCH] checkpatch: Add comment about updating Documentation/CodingStyle
On Tue, Sep 03, 2013 at 12:09:31PM -0600, Bjorn Helgaas wrote: On Mon, Sep 2, 2013 at 6:39 PM, Fengguang Wu fengguang...@intel.com wrote: Thanks! Yes I'm now running checkpatch these days because some people suggested to me that some of the checkpatch warnings do help catch real bugs. However I do try to avoid upsetting people with maybe-subjective warnings. A checkpatch report will only be sent when a small fraction of error types are detected. Comments are very welcome on how to improve this list: How hard would it be to make this configurable per-git tree? It'd be trivial to do. I think the robot is quite useful and I don't push branches very often, so I'd probably be OK with just getting all the checkpatch warnings along with the build warnings and filtering the noise myself. But I know other people have different styles and opinions. Would you tell me the git tree/branches that would like to receive all checkpatch warnings? It'll should be useful for our internal kernel team, too. Thanks, Fengguang MEMSET IN_ATOMIC UAPI_INCLUDE MALFORMED_INCLUDE SIZEOF_ADDRESS KREALLOC_ARG_REUSE EXECUTE_PERMISSIONS ERROR:BAD_SIGN_OFF LO_MACRO HI_MACRO CSYNC SSYNC HOTPLUG_SECTION INDENTED_LABEL INLINE_LOCATION STORAGE_CLASS USLEEP_RANGE UNNECESSARY_CASTS ALLOC_SIZEOF_STRUCT KREALLOC_ARG_REUSE USE_FUNC LOCKDEP EXPORTED_WORLD_WRITABLE WHITESPACE_AFTER_LINE_CONTINUATION MISSING_VMLINUX_SYMBOL NEEDLESS_IF PRINTF_L Once the decision is made to send a checkpatch error/warning, the report email will use the triggering error (the one that matters) as the email subject, with the complete output of checkpatch.pl included in email body. Thanks, Fengguang ___ Ksummit-2013-discuss mailing list ksummit-2013-disc...@lists.linuxfoundation.org https://lists.linuxfoundation.org/mailman/listinfo/ksummit-2013-discuss -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Ksummit-2013-discuss] [PATCH] checkpatch: Add comment about updating Documentation/CodingStyle
On Mon, Sep 02, 2013 at 08:16:45PM -0700, Josh Triplett wrote: > On Tue, Sep 03, 2013 at 10:46:40AM +0800, Fengguang Wu wrote: > > On Mon, Sep 02, 2013 at 06:52:45PM -0700, Joe Perches wrote: > > > On Mon, 2013-09-02 at 18:34 -0700, Josh Triplett wrote: > > > > CONFIG_EXPERIMENTAL > > > > CVS_KEYWORD > > > > > > OK, but > [...] > > Thanks for both of your suggestions! I'll add the commonly agreed ones: > > > > +INVALID_UTF8 > > +LINUX_VERSION_CODE > > +MISSING_EOF_NEWLINE > > +HEXADECIMAL_BOOLEAN_TEST > > +ALLOC_ARRAY_ARGS > > +CONST_STRUCT > > +CONSIDER_KSTRTO > > > > And remove the duplicate one (good catch, Josh!) > > > > -KREALLOC_ARG_REUSE > > You missed CONFIG_EXPERIMENTAL and CVS_KEYWORD; see above. OK, added, thanks! Cheers, Fengguang -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Ksummit-2013-discuss] [PATCH] checkpatch: Add comment about updating Documentation/CodingStyle
On Tue, Sep 03, 2013 at 10:46:40AM +0800, Fengguang Wu wrote: > On Mon, Sep 02, 2013 at 06:52:45PM -0700, Joe Perches wrote: > > On Mon, 2013-09-02 at 18:34 -0700, Josh Triplett wrote: > > > CONFIG_EXPERIMENTAL > > > CVS_KEYWORD > > > > OK, but [...] > Thanks for both of your suggestions! I'll add the commonly agreed ones: > > +INVALID_UTF8 > +LINUX_VERSION_CODE > +MISSING_EOF_NEWLINE > +HEXADECIMAL_BOOLEAN_TEST > +ALLOC_ARRAY_ARGS > +CONST_STRUCT > +CONSIDER_KSTRTO > > And remove the duplicate one (good catch, Josh!) > > -KREALLOC_ARG_REUSE You missed CONFIG_EXPERIMENTAL and CVS_KEYWORD; see above. - Josh Triplett -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Ksummit-2013-discuss] [PATCH] checkpatch: Add comment about updating Documentation/CodingStyle
On Mon, Sep 02, 2013 at 06:52:45PM -0700, Joe Perches wrote: > On Mon, 2013-09-02 at 18:34 -0700, Josh Triplett wrote: > > I'd suggest a couple more, which > > *should* always make sense, and to the best of my knowledge don't tend > > to generate false positives: > > > > C99_COMMENTS > > I don't have a problem with c99 comments. > As far as I know, Linus doesn't either. > > https://lkml.org/lkml/2012/4/16/473 > > > CONFIG_EXPERIMENTAL > > CVS_KEYWORD > > OK, but > > > ELSE_AFTER_BRACE > > I wouldn't do this one. I think > there are some false positives here. > > > GLOBAL_INITIALIZERS > > INITIALISED_STATIC > > Nor these. > > > INVALID_UTF8 > > LINUX_VERSION_CODE > > MISSING_EOF_NEWLINE > > OK I suppose. > > > PREFER_SEQ_PUTS > > PRINTK_WITHOUT_KERN_LEVEL > > There are a lot of these. > I suggest no here. > > > RETURN_PARENTHESES > > SIZEOF_PARENTHESIS > > It's in coding style, but some newish patches > do avoid them. It's a question about how noisy > you want your robot to be. I'd prefer the robot to show up only when necessary. The coding style warnings are good for the developers who actively run checkpatch.pl to make their patch better. However most are probably not suitable for a robot to send people unsolicited warnings. > > SPACE_BEFORE_TAB > > TRAILING_SEMICOLON > > TRAILING_WHITESPACE > > USE_DEVICE_INITCALL > > > USE_RELATIVE_PATH > > Having checkpatch tell people how to write changelogs > I think not a great idea. > > > These *ought* to make sense, but I don't know their false positive rates: > > > > HEXADECIMAL_BOOLEAN_TEST > > That's a good one. 0 false positives. > > > ALLOC_ARRAY_ARGS > > Yes, this would be reasonable too. > > > CONSIDER_KSTRTO > > I think orobably not. This would be a cleanup thing. Perhaps we can run it for a while, so that people at least come to aware there is a kstrto() for use. :) > > CONST_STRUCT > > OK > > > SPLIT_STRING > > I suggest no but Thanks for both of your suggestions! I'll add the commonly agreed ones: +INVALID_UTF8 +LINUX_VERSION_CODE +MISSING_EOF_NEWLINE +HEXADECIMAL_BOOLEAN_TEST +ALLOC_ARRAY_ARGS +CONST_STRUCT +CONSIDER_KSTRTO And remove the duplicate one (good catch, Josh!) -KREALLOC_ARG_REUSE Thanks, Fengguang -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Ksummit-2013-discuss] [PATCH] checkpatch: Add comment about updating Documentation/CodingStyle
On Mon, 2013-09-02 at 19:12 -0700, Josh Triplett wrote: > On Mon, Sep 02, 2013 at 06:52:45PM -0700, Joe Perches wrote: > > On Mon, 2013-09-02 at 18:34 -0700, Josh Triplett wrote: > > > I'd suggest a couple more, which > > > *should* always make sense, and to the best of my knowledge don't tend > > > to generate false positives: Hey Josh. I don't want to enable too many types of messages because the "barrier to entry" to submit patches shouldn't be so high that it discourages people. I feel mostly that many types of checkpatch messages are OK to emit, but aren't necessary to fix and people should feel that checkpatch isn't a necessary thing to silence before patches are accepted. cheers, Joe -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Ksummit-2013-discuss] [PATCH] checkpatch: Add comment about updating Documentation/CodingStyle
On Mon, Sep 02, 2013 at 06:52:45PM -0700, Joe Perches wrote: > On Mon, 2013-09-02 at 18:34 -0700, Josh Triplett wrote: > > I'd suggest a couple more, which > > *should* always make sense, and to the best of my knowledge don't tend > > to generate false positives: > > > > C99_COMMENTS > > I don't have a problem with c99 comments. > As far as I know, Linus doesn't either. > > https://lkml.org/lkml/2012/4/16/473 That doesn't look like an endorsement so much as a statement that C99 comments are less awful than the net/ special-case comment style. Documentation/CodingStyle chapter 8 says: > Linux style for comments is the C89 "/* ... */" style. > Don't use C99-style "// ..." comments. If that no longer holds true, we should remove it from CodingStyle. As far as I know, though, it still holds. In any case, it rarely comes up; most kernel code doesn't use such comments. > > CONFIG_EXPERIMENTAL > > CVS_KEYWORD > > OK, but Sure, I don't expect them to come up often. > > ELSE_AFTER_BRACE > > I wouldn't do this one. I think > there are some false positives here. Oh? What kinds of false positives have you seen? In any case, fair enough. > > GLOBAL_INITIALIZERS > > INITIALISED_STATIC > > Nor these. I don't see an obvious way for those to have false positives. What have you seen? > > INVALID_UTF8 > > LINUX_VERSION_CODE > > MISSING_EOF_NEWLINE > > OK I suppose. Not particularly critical, but uncontroversial and no false positives. > > PREFER_SEQ_PUTS > > PRINTK_WITHOUT_KERN_LEVEL > > There are a lot of these. > I suggest no here. I assume the bot only applies this to new patches, not to existing code, in which case these seem completely reasonable. New code should follow these, even if we don't mass-fix existing code. > > RETURN_PARENTHESES > > SIZEOF_PARENTHESIS > > It's in coding style, but some newish patches > do avoid them. It's a question about how noisy > you want your robot to be. These two seem reasonable to enforce on new code. I agree that they shouldn't trigger mass cleanups of existing code. > > SPACE_BEFORE_TAB > > TRAILING_SEMICOLON > > TRAILING_WHITESPACE > > USE_DEVICE_INITCALL I didn't see any comment from you on these four. Thoughts? > > USE_RELATIVE_PATH > > Having checkpatch tell people how to write changelogs > I think not a great idea. In general, sure, but that particular one seems OK. In any case, not particularly critical. > > These *ought* to make sense, but I don't know their false positive rates: > > > > HEXADECIMAL_BOOLEAN_TEST > > That's a good one. 0 false positives. Ah, good. > > ALLOC_ARRAY_ARGS > > Yes, this would be reasonable too. Excellent. > > CONSIDER_KSTRTO > > I think orobably not. This would be a cleanup thing. Even if applied to new code only? New code should use the right functions to start with. > > CONST_STRUCT > > OK Good to know; glad to hear it doesn't have false positives. > > SPLIT_STRING > > I suggest no but I can easily believe that it has too many false positives. Let's leave that one alone for now. - Josh Triplett -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Ksummit-2013-discuss] [PATCH] checkpatch: Add comment about updating Documentation/CodingStyle
On Mon, 2013-09-02 at 18:34 -0700, Josh Triplett wrote: > I'd suggest a couple more, which > *should* always make sense, and to the best of my knowledge don't tend > to generate false positives: > > C99_COMMENTS I don't have a problem with c99 comments. As far as I know, Linus doesn't either. https://lkml.org/lkml/2012/4/16/473 > CONFIG_EXPERIMENTAL > CVS_KEYWORD OK, but > ELSE_AFTER_BRACE I wouldn't do this one. I think there are some false positives here. > GLOBAL_INITIALIZERS > INITIALISED_STATIC Nor these. > INVALID_UTF8 > LINUX_VERSION_CODE > MISSING_EOF_NEWLINE OK I suppose. > PREFER_SEQ_PUTS > PRINTK_WITHOUT_KERN_LEVEL There are a lot of these. I suggest no here. > RETURN_PARENTHESES > SIZEOF_PARENTHESIS It's in coding style, but some newish patches do avoid them. It's a question about how noisy you want your robot to be. > SPACE_BEFORE_TAB > TRAILING_SEMICOLON > TRAILING_WHITESPACE > USE_DEVICE_INITCALL > USE_RELATIVE_PATH Having checkpatch tell people how to write changelogs I think not a great idea. > These *ought* to make sense, but I don't know their false positive rates: > > HEXADECIMAL_BOOLEAN_TEST That's a good one. 0 false positives. > ALLOC_ARRAY_ARGS Yes, this would be reasonable too. > CONSIDER_KSTRTO I think orobably not. This would be a cleanup thing. > CONST_STRUCT OK > SPLIT_STRING I suggest no but -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Ksummit-2013-discuss] [PATCH] checkpatch: Add comment about updating Documentation/CodingStyle
On Mon, Sep 02, 2013 at 05:47:54PM -0700, Joe Perches wrote: > On Tue, 2013-09-03 at 08:39 +0800, Fengguang Wu wrote: > > On Mon, Sep 02, 2013 at 02:11:36PM -0700, Joe Perches wrote: > [] > > > Fengguang Wu's very useful build robot > > > sends out emails on build failures. > > > I think that's great. > > > > Thanks! Yes I'm now running checkpatch these days because some people > > suggested to me that some of the checkpatch warnings do help catch > > real bugs. > > Hi Fengguang. > > I see, I don't recall receiving one of these so > it must be working just fine. Hi Joe! Log shows that one of your patch being checked earlier today: [4 days ago, Joe Perches] perf: Convert kmalloc_node(...GFP_ZERO...) to kzalloc_node() If you have more patches in some git tree that missed the check, please let me know. > > However I do try to avoid upsetting people with maybe-subjective > > warnings. A checkpatch report will only be sent when a small fraction > > of error types are detected. Comments are very welcome on how to > > improve this list: > > Your list seems reasonable. > > I might add: > > DOS_LINE_ENDINGS > MODIFIED_INCLUDE_ASM > JIFFIES_COMPARISON > ONE_SEMICOLON Yeah they all look good to have. Thanks for the suggestions again! Thanks, Fengguang -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Ksummit-2013-discuss] [PATCH] checkpatch: Add comment about updating Documentation/CodingStyle
On Tue, Sep 03, 2013 at 08:39:58AM +0800, Fengguang Wu wrote: > On Mon, Sep 02, 2013 at 02:11:36PM -0700, Joe Perches wrote: > > On Mon, 2013-09-02 at 21:50 +0100, David Howells wrote: > > > Josh Triplett wrote: > > > > > > > > There are many checkpatch rules (like semicolons) that > > > > > are not in CodingStyle. > > > > > > > > It's a rule of thumb, not a mandate. In *general*, checkpatch.pl should > > > > not be enforcing style rules that aren't documented in CodingStyle. > > > > > > Except that it becomes a mandate when someone runs it automatically > > > against > > > every one of your patches and then sends you an email for each patch it > > > finds > > > a checkpatch niggle against... > > > > I think that any robot sending such checkpatch-only > > emails should be disabled. > > > > I know of 2 email robots. > > > > Fengguang Wu's very useful build robot > > sends out emails on build failures. > > I think that's great. > > Thanks! Yes I'm now running checkpatch these days because some people > suggested to me that some of the checkpatch warnings do help catch > real bugs. > > However I do try to avoid upsetting people with maybe-subjective > warnings. A checkpatch report will only be sent when a small fraction > of error types are detected. Comments are very welcome on how to > improve this list: > > MEMSET > IN_ATOMIC > UAPI_INCLUDE > MALFORMED_INCLUDE > SIZEOF_ADDRESS > KREALLOC_ARG_REUSE > EXECUTE_PERMISSIONS > ERROR:BAD_SIGN_OFF > LO_MACRO > HI_MACRO > CSYNC > SSYNC > HOTPLUG_SECTION > INDENTED_LABEL > INLINE_LOCATION > STORAGE_CLASS > USLEEP_RANGE > UNNECESSARY_CASTS > ALLOC_SIZEOF_STRUCT > KREALLOC_ARG_REUSE > USE_FUNC > LOCKDEP > EXPORTED_WORLD_WRITABLE > WHITESPACE_AFTER_LINE_CONTINUATION > MISSING_VMLINUX_SYMBOL > NEEDLESS_IF > PRINTF_L Looks like you have KREALLOC_ARG_REUSE in that list twice. Other than that, those look sensible. I'd suggest a couple more, which *should* always make sense, and to the best of my knowledge don't tend to generate false positives: C99_COMMENTS CONFIG_EXPERIMENTAL CVS_KEYWORD ELSE_AFTER_BRACE GLOBAL_INITIALIZERS INITIALISED_STATIC INVALID_UTF8 LINUX_VERSION_CODE MISSING_EOF_NEWLINE PREFER_SEQ_PUTS PRINTK_WITHOUT_KERN_LEVEL REDUNDANT_CODE RETURN_PARENTHESES SIZEOF_PARENTHESIS SPACE_BEFORE_TAB TRAILING_SEMICOLON TRAILING_WHITESPACE USE_DEVICE_INITCALL USE_RELATIVE_PATH These *ought* to make sense, but I don't know their false positive rates: HEXADECIMAL_BOOLEAN_TEST ALLOC_ARRAY_ARGS CONSIDER_KSTRTO CONST_STRUCT SPLIT_STRING The following almost always make sense, but only on patches not yet applied to a tree: PATCH_PREFIX MODIFIED_INCLUDE_ASM CORRUPTED_PATCH NOT_UNIFIED_DIFF MISSING_SIGN_OFF - Josh Triplett -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Ksummit-2013-discuss] [PATCH] checkpatch: Add comment about updating Documentation/CodingStyle
On Mon, Sep 02, 2013 at 05:36:03PM -0700, Josh Triplett wrote: > On Tue, Sep 03, 2013 at 08:26:21AM +0800, Shilong Wang wrote: > > 2013/9/3 Joe Perches : > > > Wang Shilong > > > sent me an automated checkpatch email I > > > thought was not useful. > > > > I am sorry if i give you any trouble, i have disabled it(in fact, it > > only has run for a day!) > > I would suggest that you leave it running, but rather than sending mails > directly, have it prep the mails for you to send after manual review. > Do some careful scrutiny for false positives and cases where the change > would not improve the code, and use checkpatch's options to turn off > the more contentious warnings (like the 80-column warning). Over time, > you'll develop a set of options that produce warnings people mostly > *want* to get notified about. Good suggestions! That's exactly what I'm trying to do. And Joe kindly showed me the initial list of checkpatch error types suitable for auto notification. Coverage is good: the checkpatch robot iterates over every new commit in the 300+ git trees I collected over time. Some maintainer trees are skipped because they should already run the check. Here is the list of reports sent in the last two weeks. They are private emails directly sent to the commit author and committer. So far I've not received complaints on these unsolicited checkpatch reports. Aug 23 [netdev-next:master 200/301] WARNING: usb_free_urb(NULL) is safe this check is probably n Aug 23 [netdev-next:master 202/301] WARNING: usb_free_urb(NULL) is safe this check is probably n Aug 23 [linuxtv-media:master 321/499] ERROR: Unrecognized email address: 'Kyungmin Park http://c-faq.com/ma Aug 28 [mmotm:master 473/483] WARNING: __func__ should be used instead of gcc specific __FUNCTIO Aug 28 [kvm:queue 13/14] ERROR: Unrecognized email address: 'Gleb Natapov @g...@redhat.com>' Aug 29 [dhowells-fs:keys-devel 9/12] WARNING: labels should not be indented Aug 29 [jolsa-perf:perf/plugins2 14/20] WARNING: storage class should be at the beginning of the Aug 30 [nfs:testing 47/61] ERROR: Unrecognized email address: 'Trond Myklebust http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Ksummit-2013-discuss] [PATCH] checkpatch: Add comment about updating Documentation/CodingStyle
On Tue, 2013-09-03 at 08:39 +0800, Fengguang Wu wrote: > On Mon, Sep 02, 2013 at 02:11:36PM -0700, Joe Perches wrote: [] > > Fengguang Wu's very useful build robot > > sends out emails on build failures. > > I think that's great. > > Thanks! Yes I'm now running checkpatch these days because some people > suggested to me that some of the checkpatch warnings do help catch > real bugs. Hi Fengguang. I see, I don't recall receiving one of these so it must be working just fine. > However I do try to avoid upsetting people with maybe-subjective > warnings. A checkpatch report will only be sent when a small fraction > of error types are detected. Comments are very welcome on how to > improve this list: Your list seems reasonable. I might add: DOS_LINE_ENDINGS MODIFIED_INCLUDE_ASM JIFFIES_COMPARISON ONE_SEMICOLON -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Ksummit-2013-discuss] [PATCH] checkpatch: Add comment about updating Documentation/CodingStyle
On Mon, Sep 02, 2013 at 02:11:36PM -0700, Joe Perches wrote: > On Mon, 2013-09-02 at 21:50 +0100, David Howells wrote: > > Josh Triplett wrote: > > > > > > There are many checkpatch rules (like semicolons) that > > > > are not in CodingStyle. > > > > > > It's a rule of thumb, not a mandate. In *general*, checkpatch.pl should > > > not be enforcing style rules that aren't documented in CodingStyle. > > > > Except that it becomes a mandate when someone runs it automatically against > > every one of your patches and then sends you an email for each patch it > > finds > > a checkpatch niggle against... > > I think that any robot sending such checkpatch-only > emails should be disabled. > > I know of 2 email robots. > > Fengguang Wu's very useful build robot > sends out emails on build failures. > I think that's great. Thanks! Yes I'm now running checkpatch these days because some people suggested to me that some of the checkpatch warnings do help catch real bugs. However I do try to avoid upsetting people with maybe-subjective warnings. A checkpatch report will only be sent when a small fraction of error types are detected. Comments are very welcome on how to improve this list: MEMSET IN_ATOMIC UAPI_INCLUDE MALFORMED_INCLUDE SIZEOF_ADDRESS KREALLOC_ARG_REUSE EXECUTE_PERMISSIONS ERROR:BAD_SIGN_OFF LO_MACRO HI_MACRO CSYNC SSYNC HOTPLUG_SECTION INDENTED_LABEL INLINE_LOCATION STORAGE_CLASS USLEEP_RANGE UNNECESSARY_CASTS ALLOC_SIZEOF_STRUCT KREALLOC_ARG_REUSE USE_FUNC LOCKDEP EXPORTED_WORLD_WRITABLE WHITESPACE_AFTER_LINE_CONTINUATION MISSING_VMLINUX_SYMBOL NEEDLESS_IF PRINTF_L Once the decision is made to send a checkpatch error/warning, the report email will use the triggering error (the one that matters) as the email subject, with the complete output of checkpatch.pl included in email body. Thanks, Fengguang -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Ksummit-2013-discuss] [PATCH] checkpatch: Add comment about updating Documentation/CodingStyle
On Tue, Sep 03, 2013 at 08:26:21AM +0800, Shilong Wang wrote: > 2013/9/3 Joe Perches : > > Wang Shilong > > sent me an automated checkpatch email I > > thought was not useful. > > I am sorry if i give you any trouble, i have disabled it(in fact, it > only has run for a day!) I would suggest that you leave it running, but rather than sending mails directly, have it prep the mails for you to send after manual review. Do some careful scrutiny for false positives and cases where the change would not improve the code, and use checkpatch's options to turn off the more contentious warnings (like the 80-column warning). Over time, you'll develop a set of options that produce warnings people mostly *want* to get notified about. - Josh Triplett -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Ksummit-2013-discuss] [PATCH] checkpatch: Add comment about updating Documentation/CodingStyle
2013/9/3 Joe Perches : > On Mon, 2013-09-02 at 21:50 +0100, David Howells wrote: >> Josh Triplett wrote: >> >> > > There are many checkpatch rules (like semicolons) that >> > > are not in CodingStyle. >> > >> > It's a rule of thumb, not a mandate. In *general*, checkpatch.pl should >> > not be enforcing style rules that aren't documented in CodingStyle. >> >> Except that it becomes a mandate when someone runs it automatically against >> every one of your patches and then sends you an email for each patch it finds >> a checkpatch niggle against... Agree with this.. But using checkpatch.pl, i found there are *so many* patches that have warnings or errors. As far as i know, patches with checkpatch.pl's errors should be avoided at least unless there is a *bug* in checkpatch.pl! > > I think that any robot sending such checkpatch-only > emails should be disabled. > > I know of 2 email robots. > > Fengguang Wu's very useful build robot > sends out emails on build failures. > I think that's great. > > Wang Shilong > sent me an automated checkpatch email I > thought was not useful. I am sorry if i give you any trouble, i have disabled it(in fact, it only has run for a day!) Thanks, wang > > Does anyone know of other checkpatch robots? > > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Ksummit-2013-discuss] [PATCH] checkpatch: Add comment about updating Documentation/CodingStyle
On Mon, Sep 02, 2013 at 09:50:23PM +0100, David Howells wrote: > Josh Triplett wrote: > > > > There are many checkpatch rules (like semicolons) that > > > are not in CodingStyle. > > > > It's a rule of thumb, not a mandate. In *general*, checkpatch.pl should > > not be enforcing style rules that aren't documented in CodingStyle. > > Except that it becomes a mandate when someone runs it automatically against > every one of your patches and then sends you an email for each patch it finds > a checkpatch niggle against... I think we're talking about two different things. I wasn't talking about checkpatch.pl itself; I was talking about the idea that every style rule in checkpatch.pl should corresponding documentation in CodingStyle. That's what I was calling a rule of thumb rather than a mandate. - Josh triplett -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Ksummit-2013-discuss] [PATCH] checkpatch: Add comment about updating Documentation/CodingStyle
On Mon, 2013-09-02 at 21:50 +0100, David Howells wrote: > Josh Triplett wrote: > > > > There are many checkpatch rules (like semicolons) that > > > are not in CodingStyle. > > > > It's a rule of thumb, not a mandate. In *general*, checkpatch.pl should > > not be enforcing style rules that aren't documented in CodingStyle. > > Except that it becomes a mandate when someone runs it automatically against > every one of your patches and then sends you an email for each patch it finds > a checkpatch niggle against... I think that any robot sending such checkpatch-only emails should be disabled. I know of 2 email robots. Fengguang Wu's very useful build robot sends out emails on build failures. I think that's great. Wang Shilong sent me an automated checkpatch email I thought was not useful. Does anyone know of other checkpatch robots? -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Ksummit-2013-discuss] [PATCH] checkpatch: Add comment about updating Documentation/CodingStyle
Josh Triplett wrote: > > There are many checkpatch rules (like semicolons) that > > are not in CodingStyle. > > It's a rule of thumb, not a mandate. In *general*, checkpatch.pl should > not be enforcing style rules that aren't documented in CodingStyle. Except that it becomes a mandate when someone runs it automatically against every one of your patches and then sends you an email for each patch it finds a checkpatch niggle against... David -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Ksummit-2013-discuss] [PATCH] checkpatch: Add comment about updating Documentation/CodingStyle
On 09/02/2013 12:50 PM, Josh Triplett wrote: On Mon, Sep 02, 2013 at 11:59:27AM -0700, Joe Perches wrote: On Mon, 2013-09-02 at 15:39 -0300, Mauro Carvalho Chehab wrote: Em Mon, 2 Sep 2013 11:19:01 -0700 Josh Triplett escreveu: [] +# This file does not define the kernel coding style; Documentation/CodingStyle +# does. If you add a new style test to this file, add the corresponding style +# rule it enforces to Documentation/CodingStyle. Agreed with that. I do not. I would also add another comment there: "in case of conflicts between checkpatch.pl and Documentation/CodingStyle, the latter takes precedence." There are many checkpatch rules (like semicolons) that are not in CodingStyle. It's a rule of thumb, not a mandate. In *general*, checkpatch.pl should not be enforcing style rules that aren't documented in CodingStyle. Oddly enough, the opposite is true as well. 3.1, spaces around binary and ternary operators, is for example not enforced, presumably because it would generate too many positives. Since I like that rule, I have my private version of checkpatch.pl which does check for it. After all, it _is_ a CodingStyle rule. Guenter CodingStyle should not become some intensely detailed document that specifies the "only one true way" to write code. Any rule that maintainers are likely to enforce on patches they review should live in Documentation/CodingStyle; unwritten rules are a bad idea. Any rule that maintainers are *not* likely to enforce shouldn't go in scripts/checkpatch.pl. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Ksummit-2013-discuss] [PATCH] checkpatch: Add comment about updating Documentation/CodingStyle
On Mon, Sep 02, 2013 at 11:59:27AM -0700, Joe Perches wrote: > On Mon, 2013-09-02 at 15:39 -0300, Mauro Carvalho Chehab wrote: > > Em Mon, 2 Sep 2013 11:19:01 -0700 > > Josh Triplett escreveu: > [] > > > +# This file does not define the kernel coding style; > > > Documentation/CodingStyle > > > +# does. If you add a new style test to this file, add the corresponding > > > style > > > +# rule it enforces to Documentation/CodingStyle. > > > Agreed with that. > > I do not. > > > I would also add another comment there: "in case of > > conflicts between checkpatch.pl and Documentation/CodingStyle, the latter > > takes precedence." > > There are many checkpatch rules (like semicolons) that > are not in CodingStyle. It's a rule of thumb, not a mandate. In *general*, checkpatch.pl should not be enforcing style rules that aren't documented in CodingStyle. > CodingStyle should not become some intensely detailed > document that specifies the "only one true way" to > write code. Any rule that maintainers are likely to enforce on patches they review should live in Documentation/CodingStyle; unwritten rules are a bad idea. Any rule that maintainers are *not* likely to enforce shouldn't go in scripts/checkpatch.pl. - Josh Triplett -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Ksummit-2013-discuss] [PATCH] checkpatch: Add comment about updating Documentation/CodingStyle
Em Mon, 02 Sep 2013 11:59:27 -0700 Joe Perches escreveu: > On Mon, 2013-09-02 at 15:39 -0300, Mauro Carvalho Chehab wrote: > > Em Mon, 2 Sep 2013 11:19:01 -0700 > > Josh Triplett escreveu: > [] > > > +# This file does not define the kernel coding style; > > > Documentation/CodingStyle > > > +# does. If you add a new style test to this file, add the corresponding > > > style > > > +# rule it enforces to Documentation/CodingStyle. > > > Agreed with that. > > I do not. > > > I would also add another comment there: "in case of > > conflicts between checkpatch.pl and Documentation/CodingStyle, the latter > > takes precedence." > > There are many checkpatch rules (like semicolons) that > are not in CodingStyle. Well, document them there, please. > CodingStyle should not become some intensely detailed > document that specifies the "only one true way" to > write code. There will always be things that will be freed to the programmer to use, but CodingStyle should reflect what is the coding style we're adopting at the Kernel, or putting it on another way: what things will make the patch to be rejected because of its style. > I also think checkpatch should not be used by robots > to determine that patches are bad or unacceptable. > > checkpatch is just a dumb little tool that has some > utility but as Dan Carpenter once said, "it's less > sentient than a squirrel". > > People should always use their own taste before > relying on dumb tools. That's easier said than done. There are lots of stupid changes that are done by developers (like enforcing 80 cols whitespace breaks) just because of the checkpatch warnings. That happens before those patches got sent to the ML's, as most people know that maintainers will curse them if the coding style is crap[1]. [1] BTW, most of the time that checkpatch complains, the code is really crap. In any case, Documentation/CodingStyle is the reference document that maintainers and coders should use to know that a code is following the style (and not checkpatch). So, it requires updates when new CodingStyle enforcements are created. In the specific example pointed by David, if "extern", will start to become a bad word that should be avoided, that should be documented there at CodingStyle, and checkpatch should just be the dumb monkey that will check that. Cheers, Mauro -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Ksummit-2013-discuss] [PATCH] checkpatch: Add comment about updating Documentation/CodingStyle
On Mon, Sep 02, 2013 at 03:39:45PM -0300, Mauro Carvalho Chehab wrote: > Em Mon, 2 Sep 2013 11:19:01 -0700 > Josh Triplett escreveu: > > > Patches to checkpatch that add new style rules should also change > > Documentation/CodingStyle to document those new style rules; add a > > comment to that effect to the top of scripts/checkpatch.pl. > > Well, you forgot to c/c LKML on this patch; I think that KS2013 is not the > proper list to review this patch ;) My mail went to LKML; it had: To: linux-kernel@vger.kernel.org > > Signed-off-by: Josh Triplett > > --- > > scripts/checkpatch.pl | 4 > > 1 file changed, 4 insertions(+) > > > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl > > index 2ee9eb7..ba65ea6 100755 > > --- a/scripts/checkpatch.pl > > +++ b/scripts/checkpatch.pl > > @@ -4,6 +4,10 @@ > > # (c) 2007,2008, Andy Whitcroft (new conditions, test > > suite) > > # (c) 2008-2010 Andy Whitcroft > > # Licensed under the terms of the GNU GPL License version 2 > > +# > > +# This file does not define the kernel coding style; > > Documentation/CodingStyle > > +# does. If you add a new style test to this file, add the corresponding > > style > > +# rule it enforces to Documentation/CodingStyle. > > > > Agreed with that. I would also add another comment there: "in case of > conflicts between checkpatch.pl and Documentation/CodingStyle, the latter > takes precedence." Good point. > Anyway, > > Acked-by: Mauro Carvalho Chehab Thanks. - Josh Triplett -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Ksummit-2013-discuss] [PATCH] checkpatch: Add comment about updating Documentation/CodingStyle
On Mon, 2013-09-02 at 15:39 -0300, Mauro Carvalho Chehab wrote: > Em Mon, 2 Sep 2013 11:19:01 -0700 > Josh Triplett escreveu: [] > > +# This file does not define the kernel coding style; > > Documentation/CodingStyle > > +# does. If you add a new style test to this file, add the corresponding > > style > > +# rule it enforces to Documentation/CodingStyle. > Agreed with that. I do not. > I would also add another comment there: "in case of > conflicts between checkpatch.pl and Documentation/CodingStyle, the latter > takes precedence." There are many checkpatch rules (like semicolons) that are not in CodingStyle. CodingStyle should not become some intensely detailed document that specifies the "only one true way" to write code. I also think checkpatch should not be used by robots to determine that patches are bad or unacceptable. checkpatch is just a dumb little tool that has some utility but as Dan Carpenter once said, "it's less sentient than a squirrel". People should always use their own taste before relying on dumb tools. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Ksummit-2013-discuss] [PATCH] checkpatch: Add comment about updating Documentation/CodingStyle
Em Mon, 2 Sep 2013 11:19:01 -0700 Josh Triplett escreveu: > Patches to checkpatch that add new style rules should also change > Documentation/CodingStyle to document those new style rules; add a > comment to that effect to the top of scripts/checkpatch.pl. Well, you forgot to c/c LKML on this patch; I think that KS2013 is not the proper list to review this patch ;) > > Signed-off-by: Josh Triplett > --- > scripts/checkpatch.pl | 4 > 1 file changed, 4 insertions(+) > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl > index 2ee9eb7..ba65ea6 100755 > --- a/scripts/checkpatch.pl > +++ b/scripts/checkpatch.pl > @@ -4,6 +4,10 @@ > # (c) 2007,2008, Andy Whitcroft (new conditions, test > suite) > # (c) 2008-2010 Andy Whitcroft > # Licensed under the terms of the GNU GPL License version 2 > +# > +# This file does not define the kernel coding style; > Documentation/CodingStyle > +# does. If you add a new style test to this file, add the corresponding > style > +# rule it enforces to Documentation/CodingStyle. > Agreed with that. I would also add another comment there: "in case of conflicts between checkpatch.pl and Documentation/CodingStyle, the latter takes precedence." Anyway, Acked-by: Mauro Carvalho Chehab > use strict; > use POSIX; Regard -- Cheers, Mauro -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Ksummit-2013-discuss] [PATCH] checkpatch: Add comment about updating Documentation/CodingStyle
Em Mon, 2 Sep 2013 11:19:01 -0700 Josh Triplett j...@joshtriplett.org escreveu: Patches to checkpatch that add new style rules should also change Documentation/CodingStyle to document those new style rules; add a comment to that effect to the top of scripts/checkpatch.pl. Well, you forgot to c/c LKML on this patch; I think that KS2013 is not the proper list to review this patch ;) Signed-off-by: Josh Triplett j...@joshtriplett.org --- scripts/checkpatch.pl | 4 1 file changed, 4 insertions(+) diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl index 2ee9eb7..ba65ea6 100755 --- a/scripts/checkpatch.pl +++ b/scripts/checkpatch.pl @@ -4,6 +4,10 @@ # (c) 2007,2008, Andy Whitcroft a...@uk.ibm.com (new conditions, test suite) # (c) 2008-2010 Andy Whitcroft a...@canonical.com # Licensed under the terms of the GNU GPL License version 2 +# +# This file does not define the kernel coding style; Documentation/CodingStyle +# does. If you add a new style test to this file, add the corresponding style +# rule it enforces to Documentation/CodingStyle. Agreed with that. I would also add another comment there: in case of conflicts between checkpatch.pl and Documentation/CodingStyle, the latter takes precedence. Anyway, Acked-by: Mauro Carvalho Chehab m.che...@samsung.com use strict; use POSIX; Regard -- Cheers, Mauro -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Ksummit-2013-discuss] [PATCH] checkpatch: Add comment about updating Documentation/CodingStyle
On Mon, 2013-09-02 at 15:39 -0300, Mauro Carvalho Chehab wrote: Em Mon, 2 Sep 2013 11:19:01 -0700 Josh Triplett j...@joshtriplett.org escreveu: [] +# This file does not define the kernel coding style; Documentation/CodingStyle +# does. If you add a new style test to this file, add the corresponding style +# rule it enforces to Documentation/CodingStyle. Agreed with that. I do not. I would also add another comment there: in case of conflicts between checkpatch.pl and Documentation/CodingStyle, the latter takes precedence. There are many checkpatch rules (like semicolons) that are not in CodingStyle. CodingStyle should not become some intensely detailed document that specifies the only one true way to write code. I also think checkpatch should not be used by robots to determine that patches are bad or unacceptable. checkpatch is just a dumb little tool that has some utility but as Dan Carpenter once said, it's less sentient than a squirrel. People should always use their own taste before relying on dumb tools. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Ksummit-2013-discuss] [PATCH] checkpatch: Add comment about updating Documentation/CodingStyle
On Mon, Sep 02, 2013 at 03:39:45PM -0300, Mauro Carvalho Chehab wrote: Em Mon, 2 Sep 2013 11:19:01 -0700 Josh Triplett j...@joshtriplett.org escreveu: Patches to checkpatch that add new style rules should also change Documentation/CodingStyle to document those new style rules; add a comment to that effect to the top of scripts/checkpatch.pl. Well, you forgot to c/c LKML on this patch; I think that KS2013 is not the proper list to review this patch ;) My mail went to LKML; it had: To: linux-kernel@vger.kernel.org Signed-off-by: Josh Triplett j...@joshtriplett.org --- scripts/checkpatch.pl | 4 1 file changed, 4 insertions(+) diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl index 2ee9eb7..ba65ea6 100755 --- a/scripts/checkpatch.pl +++ b/scripts/checkpatch.pl @@ -4,6 +4,10 @@ # (c) 2007,2008, Andy Whitcroft a...@uk.ibm.com (new conditions, test suite) # (c) 2008-2010 Andy Whitcroft a...@canonical.com # Licensed under the terms of the GNU GPL License version 2 +# +# This file does not define the kernel coding style; Documentation/CodingStyle +# does. If you add a new style test to this file, add the corresponding style +# rule it enforces to Documentation/CodingStyle. Agreed with that. I would also add another comment there: in case of conflicts between checkpatch.pl and Documentation/CodingStyle, the latter takes precedence. Good point. Anyway, Acked-by: Mauro Carvalho Chehab m.che...@samsung.com Thanks. - Josh Triplett -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Ksummit-2013-discuss] [PATCH] checkpatch: Add comment about updating Documentation/CodingStyle
Em Mon, 02 Sep 2013 11:59:27 -0700 Joe Perches j...@perches.com escreveu: On Mon, 2013-09-02 at 15:39 -0300, Mauro Carvalho Chehab wrote: Em Mon, 2 Sep 2013 11:19:01 -0700 Josh Triplett j...@joshtriplett.org escreveu: [] +# This file does not define the kernel coding style; Documentation/CodingStyle +# does. If you add a new style test to this file, add the corresponding style +# rule it enforces to Documentation/CodingStyle. Agreed with that. I do not. I would also add another comment there: in case of conflicts between checkpatch.pl and Documentation/CodingStyle, the latter takes precedence. There are many checkpatch rules (like semicolons) that are not in CodingStyle. Well, document them there, please. CodingStyle should not become some intensely detailed document that specifies the only one true way to write code. There will always be things that will be freed to the programmer to use, but CodingStyle should reflect what is the coding style we're adopting at the Kernel, or putting it on another way: what things will make the patch to be rejected because of its style. I also think checkpatch should not be used by robots to determine that patches are bad or unacceptable. checkpatch is just a dumb little tool that has some utility but as Dan Carpenter once said, it's less sentient than a squirrel. People should always use their own taste before relying on dumb tools. That's easier said than done. There are lots of stupid changes that are done by developers (like enforcing 80 cols whitespace breaks) just because of the checkpatch warnings. That happens before those patches got sent to the ML's, as most people know that maintainers will curse them if the coding style is crap[1]. [1] BTW, most of the time that checkpatch complains, the code is really crap. In any case, Documentation/CodingStyle is the reference document that maintainers and coders should use to know that a code is following the style (and not checkpatch). So, it requires updates when new CodingStyle enforcements are created. In the specific example pointed by David, if extern, will start to become a bad word that should be avoided, that should be documented there at CodingStyle, and checkpatch should just be the dumb monkey that will check that. Cheers, Mauro -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Ksummit-2013-discuss] [PATCH] checkpatch: Add comment about updating Documentation/CodingStyle
On Mon, Sep 02, 2013 at 11:59:27AM -0700, Joe Perches wrote: On Mon, 2013-09-02 at 15:39 -0300, Mauro Carvalho Chehab wrote: Em Mon, 2 Sep 2013 11:19:01 -0700 Josh Triplett j...@joshtriplett.org escreveu: [] +# This file does not define the kernel coding style; Documentation/CodingStyle +# does. If you add a new style test to this file, add the corresponding style +# rule it enforces to Documentation/CodingStyle. Agreed with that. I do not. I would also add another comment there: in case of conflicts between checkpatch.pl and Documentation/CodingStyle, the latter takes precedence. There are many checkpatch rules (like semicolons) that are not in CodingStyle. It's a rule of thumb, not a mandate. In *general*, checkpatch.pl should not be enforcing style rules that aren't documented in CodingStyle. CodingStyle should not become some intensely detailed document that specifies the only one true way to write code. Any rule that maintainers are likely to enforce on patches they review should live in Documentation/CodingStyle; unwritten rules are a bad idea. Any rule that maintainers are *not* likely to enforce shouldn't go in scripts/checkpatch.pl. - Josh Triplett -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Ksummit-2013-discuss] [PATCH] checkpatch: Add comment about updating Documentation/CodingStyle
On 09/02/2013 12:50 PM, Josh Triplett wrote: On Mon, Sep 02, 2013 at 11:59:27AM -0700, Joe Perches wrote: On Mon, 2013-09-02 at 15:39 -0300, Mauro Carvalho Chehab wrote: Em Mon, 2 Sep 2013 11:19:01 -0700 Josh Triplett j...@joshtriplett.org escreveu: [] +# This file does not define the kernel coding style; Documentation/CodingStyle +# does. If you add a new style test to this file, add the corresponding style +# rule it enforces to Documentation/CodingStyle. Agreed with that. I do not. I would also add another comment there: in case of conflicts between checkpatch.pl and Documentation/CodingStyle, the latter takes precedence. There are many checkpatch rules (like semicolons) that are not in CodingStyle. It's a rule of thumb, not a mandate. In *general*, checkpatch.pl should not be enforcing style rules that aren't documented in CodingStyle. Oddly enough, the opposite is true as well. 3.1, spaces around binary and ternary operators, is for example not enforced, presumably because it would generate too many positives. Since I like that rule, I have my private version of checkpatch.pl which does check for it. After all, it _is_ a CodingStyle rule. Guenter CodingStyle should not become some intensely detailed document that specifies the only one true way to write code. Any rule that maintainers are likely to enforce on patches they review should live in Documentation/CodingStyle; unwritten rules are a bad idea. Any rule that maintainers are *not* likely to enforce shouldn't go in scripts/checkpatch.pl. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Ksummit-2013-discuss] [PATCH] checkpatch: Add comment about updating Documentation/CodingStyle
Josh Triplett j...@joshtriplett.org wrote: There are many checkpatch rules (like semicolons) that are not in CodingStyle. It's a rule of thumb, not a mandate. In *general*, checkpatch.pl should not be enforcing style rules that aren't documented in CodingStyle. Except that it becomes a mandate when someone runs it automatically against every one of your patches and then sends you an email for each patch it finds a checkpatch niggle against... David -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Ksummit-2013-discuss] [PATCH] checkpatch: Add comment about updating Documentation/CodingStyle
On Mon, 2013-09-02 at 21:50 +0100, David Howells wrote: Josh Triplett j...@joshtriplett.org wrote: There are many checkpatch rules (like semicolons) that are not in CodingStyle. It's a rule of thumb, not a mandate. In *general*, checkpatch.pl should not be enforcing style rules that aren't documented in CodingStyle. Except that it becomes a mandate when someone runs it automatically against every one of your patches and then sends you an email for each patch it finds a checkpatch niggle against... I think that any robot sending such checkpatch-only emails should be disabled. I know of 2 email robots. Fengguang Wu's very useful build robot sends out emails on build failures. I think that's great. Wang Shilong wangshilong1...@gmail.com sent me an automated checkpatch email I thought was not useful. Does anyone know of other checkpatch robots? -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Ksummit-2013-discuss] [PATCH] checkpatch: Add comment about updating Documentation/CodingStyle
On Mon, Sep 02, 2013 at 09:50:23PM +0100, David Howells wrote: Josh Triplett j...@joshtriplett.org wrote: There are many checkpatch rules (like semicolons) that are not in CodingStyle. It's a rule of thumb, not a mandate. In *general*, checkpatch.pl should not be enforcing style rules that aren't documented in CodingStyle. Except that it becomes a mandate when someone runs it automatically against every one of your patches and then sends you an email for each patch it finds a checkpatch niggle against... I think we're talking about two different things. I wasn't talking about checkpatch.pl itself; I was talking about the idea that every style rule in checkpatch.pl should corresponding documentation in CodingStyle. That's what I was calling a rule of thumb rather than a mandate. - Josh triplett -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Ksummit-2013-discuss] [PATCH] checkpatch: Add comment about updating Documentation/CodingStyle
2013/9/3 Joe Perches j...@perches.com: On Mon, 2013-09-02 at 21:50 +0100, David Howells wrote: Josh Triplett j...@joshtriplett.org wrote: There are many checkpatch rules (like semicolons) that are not in CodingStyle. It's a rule of thumb, not a mandate. In *general*, checkpatch.pl should not be enforcing style rules that aren't documented in CodingStyle. Except that it becomes a mandate when someone runs it automatically against every one of your patches and then sends you an email for each patch it finds a checkpatch niggle against... Agree with this.. But using checkpatch.pl, i found there are *so many* patches that have warnings or errors. As far as i know, patches with checkpatch.pl's errors should be avoided at least unless there is a *bug* in checkpatch.pl! I think that any robot sending such checkpatch-only emails should be disabled. I know of 2 email robots. Fengguang Wu's very useful build robot sends out emails on build failures. I think that's great. Wang Shilong wangshilong1...@gmail.com sent me an automated checkpatch email I thought was not useful. I am sorry if i give you any trouble, i have disabled it(in fact, it only has run for a day!) Thanks, wang Does anyone know of other checkpatch robots? -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Ksummit-2013-discuss] [PATCH] checkpatch: Add comment about updating Documentation/CodingStyle
On Tue, Sep 03, 2013 at 08:26:21AM +0800, Shilong Wang wrote: 2013/9/3 Joe Perches j...@perches.com: Wang Shilong wangshilong1...@gmail.com sent me an automated checkpatch email I thought was not useful. I am sorry if i give you any trouble, i have disabled it(in fact, it only has run for a day!) I would suggest that you leave it running, but rather than sending mails directly, have it prep the mails for you to send after manual review. Do some careful scrutiny for false positives and cases where the change would not improve the code, and use checkpatch's options to turn off the more contentious warnings (like the 80-column warning). Over time, you'll develop a set of options that produce warnings people mostly *want* to get notified about. - Josh Triplett -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Ksummit-2013-discuss] [PATCH] checkpatch: Add comment about updating Documentation/CodingStyle
On Mon, Sep 02, 2013 at 02:11:36PM -0700, Joe Perches wrote: On Mon, 2013-09-02 at 21:50 +0100, David Howells wrote: Josh Triplett j...@joshtriplett.org wrote: There are many checkpatch rules (like semicolons) that are not in CodingStyle. It's a rule of thumb, not a mandate. In *general*, checkpatch.pl should not be enforcing style rules that aren't documented in CodingStyle. Except that it becomes a mandate when someone runs it automatically against every one of your patches and then sends you an email for each patch it finds a checkpatch niggle against... I think that any robot sending such checkpatch-only emails should be disabled. I know of 2 email robots. Fengguang Wu's very useful build robot sends out emails on build failures. I think that's great. Thanks! Yes I'm now running checkpatch these days because some people suggested to me that some of the checkpatch warnings do help catch real bugs. However I do try to avoid upsetting people with maybe-subjective warnings. A checkpatch report will only be sent when a small fraction of error types are detected. Comments are very welcome on how to improve this list: MEMSET IN_ATOMIC UAPI_INCLUDE MALFORMED_INCLUDE SIZEOF_ADDRESS KREALLOC_ARG_REUSE EXECUTE_PERMISSIONS ERROR:BAD_SIGN_OFF LO_MACRO HI_MACRO CSYNC SSYNC HOTPLUG_SECTION INDENTED_LABEL INLINE_LOCATION STORAGE_CLASS USLEEP_RANGE UNNECESSARY_CASTS ALLOC_SIZEOF_STRUCT KREALLOC_ARG_REUSE USE_FUNC LOCKDEP EXPORTED_WORLD_WRITABLE WHITESPACE_AFTER_LINE_CONTINUATION MISSING_VMLINUX_SYMBOL NEEDLESS_IF PRINTF_L Once the decision is made to send a checkpatch error/warning, the report email will use the triggering error (the one that matters) as the email subject, with the complete output of checkpatch.pl included in email body. Thanks, Fengguang -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Ksummit-2013-discuss] [PATCH] checkpatch: Add comment about updating Documentation/CodingStyle
On Tue, 2013-09-03 at 08:39 +0800, Fengguang Wu wrote: On Mon, Sep 02, 2013 at 02:11:36PM -0700, Joe Perches wrote: [] Fengguang Wu's very useful build robot sends out emails on build failures. I think that's great. Thanks! Yes I'm now running checkpatch these days because some people suggested to me that some of the checkpatch warnings do help catch real bugs. Hi Fengguang. I see, I don't recall receiving one of these so it must be working just fine. However I do try to avoid upsetting people with maybe-subjective warnings. A checkpatch report will only be sent when a small fraction of error types are detected. Comments are very welcome on how to improve this list: Your list seems reasonable. I might add: DOS_LINE_ENDINGS MODIFIED_INCLUDE_ASM JIFFIES_COMPARISON ONE_SEMICOLON -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Ksummit-2013-discuss] [PATCH] checkpatch: Add comment about updating Documentation/CodingStyle
On Mon, Sep 02, 2013 at 05:36:03PM -0700, Josh Triplett wrote: On Tue, Sep 03, 2013 at 08:26:21AM +0800, Shilong Wang wrote: 2013/9/3 Joe Perches j...@perches.com: Wang Shilong wangshilong1...@gmail.com sent me an automated checkpatch email I thought was not useful. I am sorry if i give you any trouble, i have disabled it(in fact, it only has run for a day!) I would suggest that you leave it running, but rather than sending mails directly, have it prep the mails for you to send after manual review. Do some careful scrutiny for false positives and cases where the change would not improve the code, and use checkpatch's options to turn off the more contentious warnings (like the 80-column warning). Over time, you'll develop a set of options that produce warnings people mostly *want* to get notified about. Good suggestions! That's exactly what I'm trying to do. And Joe kindly showed me the initial list of checkpatch error types suitable for auto notification. Coverage is good: the checkpatch robot iterates over every new commit in the 300+ git trees I collected over time. Some maintainer trees are skipped because they should already run the check. Here is the list of reports sent in the last two weeks. They are private emails directly sent to the commit author and committer. So far I've not received complaints on these unsolicited checkpatch reports. Aug 23 [netdev-next:master 200/301] WARNING: usb_free_urb(NULL) is safe this check is probably n Aug 23 [netdev-next:master 202/301] WARNING: usb_free_urb(NULL) is safe this check is probably n Aug 23 [linuxtv-media:master 321/499] ERROR: Unrecognized email address: 'Kyungmin Park kyungmi Aug 23 [linuxtv-media:master 322/499] ERROR: Unrecognized email address: 'Kyungmin Park kyungmi Aug 24 [xlnx:master-next 32/53] WARNING: unnecessary cast may hide bugs, see http://c-faq.com/ma Aug 28 [mmotm:master 473/483] WARNING: __func__ should be used instead of gcc specific __FUNCTIO Aug 28 [kvm:queue 13/14] ERROR: Unrecognized email address: 'Gleb Natapov @g...@redhat.com' Aug 29 [dhowells-fs:keys-devel 9/12] WARNING: labels should not be indented Aug 29 [jolsa-perf:perf/plugins2 14/20] WARNING: storage class should be at the beginning of the Aug 30 [nfs:testing 47/61] ERROR: Unrecognized email address: 'Trond Myklebust Trond.Myklebust@ Aug 31 [josef-btrfs:master 74/135] WARNING: kfree(NULL) is safe this check is probably not requi Aug 31 [jolsa-perf:perf/toggle6 6/8] WARNING: kfree(NULL) is safe this check is probably not req Sep 01 [jolsa-perf:perf/core_plugins 14/24] WARNING: storage class should be at the beginning of Thanks, Fengguang -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Ksummit-2013-discuss] [PATCH] checkpatch: Add comment about updating Documentation/CodingStyle
On Tue, Sep 03, 2013 at 08:39:58AM +0800, Fengguang Wu wrote: On Mon, Sep 02, 2013 at 02:11:36PM -0700, Joe Perches wrote: On Mon, 2013-09-02 at 21:50 +0100, David Howells wrote: Josh Triplett j...@joshtriplett.org wrote: There are many checkpatch rules (like semicolons) that are not in CodingStyle. It's a rule of thumb, not a mandate. In *general*, checkpatch.pl should not be enforcing style rules that aren't documented in CodingStyle. Except that it becomes a mandate when someone runs it automatically against every one of your patches and then sends you an email for each patch it finds a checkpatch niggle against... I think that any robot sending such checkpatch-only emails should be disabled. I know of 2 email robots. Fengguang Wu's very useful build robot sends out emails on build failures. I think that's great. Thanks! Yes I'm now running checkpatch these days because some people suggested to me that some of the checkpatch warnings do help catch real bugs. However I do try to avoid upsetting people with maybe-subjective warnings. A checkpatch report will only be sent when a small fraction of error types are detected. Comments are very welcome on how to improve this list: MEMSET IN_ATOMIC UAPI_INCLUDE MALFORMED_INCLUDE SIZEOF_ADDRESS KREALLOC_ARG_REUSE EXECUTE_PERMISSIONS ERROR:BAD_SIGN_OFF LO_MACRO HI_MACRO CSYNC SSYNC HOTPLUG_SECTION INDENTED_LABEL INLINE_LOCATION STORAGE_CLASS USLEEP_RANGE UNNECESSARY_CASTS ALLOC_SIZEOF_STRUCT KREALLOC_ARG_REUSE USE_FUNC LOCKDEP EXPORTED_WORLD_WRITABLE WHITESPACE_AFTER_LINE_CONTINUATION MISSING_VMLINUX_SYMBOL NEEDLESS_IF PRINTF_L Looks like you have KREALLOC_ARG_REUSE in that list twice. Other than that, those look sensible. I'd suggest a couple more, which *should* always make sense, and to the best of my knowledge don't tend to generate false positives: C99_COMMENTS CONFIG_EXPERIMENTAL CVS_KEYWORD ELSE_AFTER_BRACE GLOBAL_INITIALIZERS INITIALISED_STATIC INVALID_UTF8 LINUX_VERSION_CODE MISSING_EOF_NEWLINE PREFER_SEQ_PUTS PRINTK_WITHOUT_KERN_LEVEL REDUNDANT_CODE RETURN_PARENTHESES SIZEOF_PARENTHESIS SPACE_BEFORE_TAB TRAILING_SEMICOLON TRAILING_WHITESPACE USE_DEVICE_INITCALL USE_RELATIVE_PATH These *ought* to make sense, but I don't know their false positive rates: HEXADECIMAL_BOOLEAN_TEST ALLOC_ARRAY_ARGS CONSIDER_KSTRTO CONST_STRUCT SPLIT_STRING The following almost always make sense, but only on patches not yet applied to a tree: PATCH_PREFIX MODIFIED_INCLUDE_ASM CORRUPTED_PATCH NOT_UNIFIED_DIFF MISSING_SIGN_OFF - Josh Triplett -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Ksummit-2013-discuss] [PATCH] checkpatch: Add comment about updating Documentation/CodingStyle
On Mon, Sep 02, 2013 at 05:47:54PM -0700, Joe Perches wrote: On Tue, 2013-09-03 at 08:39 +0800, Fengguang Wu wrote: On Mon, Sep 02, 2013 at 02:11:36PM -0700, Joe Perches wrote: [] Fengguang Wu's very useful build robot sends out emails on build failures. I think that's great. Thanks! Yes I'm now running checkpatch these days because some people suggested to me that some of the checkpatch warnings do help catch real bugs. Hi Fengguang. I see, I don't recall receiving one of these so it must be working just fine. Hi Joe! Log shows that one of your patch being checked earlier today: [4 days ago, Joe Perches] perf: Convert kmalloc_node(...GFP_ZERO...) to kzalloc_node() If you have more patches in some git tree that missed the check, please let me know. However I do try to avoid upsetting people with maybe-subjective warnings. A checkpatch report will only be sent when a small fraction of error types are detected. Comments are very welcome on how to improve this list: Your list seems reasonable. I might add: DOS_LINE_ENDINGS MODIFIED_INCLUDE_ASM JIFFIES_COMPARISON ONE_SEMICOLON Yeah they all look good to have. Thanks for the suggestions again! Thanks, Fengguang -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Ksummit-2013-discuss] [PATCH] checkpatch: Add comment about updating Documentation/CodingStyle
On Mon, 2013-09-02 at 18:34 -0700, Josh Triplett wrote: I'd suggest a couple more, which *should* always make sense, and to the best of my knowledge don't tend to generate false positives: C99_COMMENTS I don't have a problem with c99 comments. As far as I know, Linus doesn't either. https://lkml.org/lkml/2012/4/16/473 CONFIG_EXPERIMENTAL CVS_KEYWORD OK, but shrug ELSE_AFTER_BRACE I wouldn't do this one. I think there are some false positives here. GLOBAL_INITIALIZERS INITIALISED_STATIC Nor these. INVALID_UTF8 LINUX_VERSION_CODE MISSING_EOF_NEWLINE OK I suppose. PREFER_SEQ_PUTS PRINTK_WITHOUT_KERN_LEVEL There are a lot of these. I suggest no here. RETURN_PARENTHESES SIZEOF_PARENTHESIS It's in coding style, but some newish patches do avoid them. It's a question about how noisy you want your robot to be. SPACE_BEFORE_TAB TRAILING_SEMICOLON TRAILING_WHITESPACE USE_DEVICE_INITCALL USE_RELATIVE_PATH Having checkpatch tell people how to write changelogs I think not a great idea. These *ought* to make sense, but I don't know their false positive rates: HEXADECIMAL_BOOLEAN_TEST That's a good one. 0 false positives. ALLOC_ARRAY_ARGS Yes, this would be reasonable too. CONSIDER_KSTRTO I think orobably not. This would be a cleanup thing. CONST_STRUCT OK SPLIT_STRING I suggest no but shrug -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Ksummit-2013-discuss] [PATCH] checkpatch: Add comment about updating Documentation/CodingStyle
On Mon, Sep 02, 2013 at 06:52:45PM -0700, Joe Perches wrote: On Mon, 2013-09-02 at 18:34 -0700, Josh Triplett wrote: I'd suggest a couple more, which *should* always make sense, and to the best of my knowledge don't tend to generate false positives: C99_COMMENTS I don't have a problem with c99 comments. As far as I know, Linus doesn't either. https://lkml.org/lkml/2012/4/16/473 That doesn't look like an endorsement so much as a statement that C99 comments are less awful than the net/ special-case comment style. Documentation/CodingStyle chapter 8 says: Linux style for comments is the C89 /* ... */ style. Don't use C99-style // ... comments. If that no longer holds true, we should remove it from CodingStyle. As far as I know, though, it still holds. In any case, it rarely comes up; most kernel code doesn't use such comments. CONFIG_EXPERIMENTAL CVS_KEYWORD OK, but shrug Sure, I don't expect them to come up often. ELSE_AFTER_BRACE I wouldn't do this one. I think there are some false positives here. Oh? What kinds of false positives have you seen? In any case, fair enough. GLOBAL_INITIALIZERS INITIALISED_STATIC Nor these. I don't see an obvious way for those to have false positives. What have you seen? INVALID_UTF8 LINUX_VERSION_CODE MISSING_EOF_NEWLINE OK I suppose. Not particularly critical, but uncontroversial and no false positives. PREFER_SEQ_PUTS PRINTK_WITHOUT_KERN_LEVEL There are a lot of these. I suggest no here. I assume the bot only applies this to new patches, not to existing code, in which case these seem completely reasonable. New code should follow these, even if we don't mass-fix existing code. RETURN_PARENTHESES SIZEOF_PARENTHESIS It's in coding style, but some newish patches do avoid them. It's a question about how noisy you want your robot to be. These two seem reasonable to enforce on new code. I agree that they shouldn't trigger mass cleanups of existing code. SPACE_BEFORE_TAB TRAILING_SEMICOLON TRAILING_WHITESPACE USE_DEVICE_INITCALL I didn't see any comment from you on these four. Thoughts? USE_RELATIVE_PATH Having checkpatch tell people how to write changelogs I think not a great idea. In general, sure, but that particular one seems OK. In any case, not particularly critical. These *ought* to make sense, but I don't know their false positive rates: HEXADECIMAL_BOOLEAN_TEST That's a good one. 0 false positives. Ah, good. ALLOC_ARRAY_ARGS Yes, this would be reasonable too. Excellent. CONSIDER_KSTRTO I think orobably not. This would be a cleanup thing. Even if applied to new code only? New code should use the right functions to start with. CONST_STRUCT OK Good to know; glad to hear it doesn't have false positives. SPLIT_STRING I suggest no but shrug I can easily believe that it has too many false positives. Let's leave that one alone for now. - Josh Triplett -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Ksummit-2013-discuss] [PATCH] checkpatch: Add comment about updating Documentation/CodingStyle
On Mon, 2013-09-02 at 19:12 -0700, Josh Triplett wrote: On Mon, Sep 02, 2013 at 06:52:45PM -0700, Joe Perches wrote: On Mon, 2013-09-02 at 18:34 -0700, Josh Triplett wrote: I'd suggest a couple more, which *should* always make sense, and to the best of my knowledge don't tend to generate false positives: Hey Josh. I don't want to enable too many types of messages because the barrier to entry to submit patches shouldn't be so high that it discourages people. I feel mostly that many types of checkpatch messages are OK to emit, but aren't necessary to fix and people should feel that checkpatch isn't a necessary thing to silence before patches are accepted. cheers, Joe -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Ksummit-2013-discuss] [PATCH] checkpatch: Add comment about updating Documentation/CodingStyle
On Mon, Sep 02, 2013 at 06:52:45PM -0700, Joe Perches wrote: On Mon, 2013-09-02 at 18:34 -0700, Josh Triplett wrote: I'd suggest a couple more, which *should* always make sense, and to the best of my knowledge don't tend to generate false positives: C99_COMMENTS I don't have a problem with c99 comments. As far as I know, Linus doesn't either. https://lkml.org/lkml/2012/4/16/473 CONFIG_EXPERIMENTAL CVS_KEYWORD OK, but shrug ELSE_AFTER_BRACE I wouldn't do this one. I think there are some false positives here. GLOBAL_INITIALIZERS INITIALISED_STATIC Nor these. INVALID_UTF8 LINUX_VERSION_CODE MISSING_EOF_NEWLINE OK I suppose. PREFER_SEQ_PUTS PRINTK_WITHOUT_KERN_LEVEL There are a lot of these. I suggest no here. RETURN_PARENTHESES SIZEOF_PARENTHESIS It's in coding style, but some newish patches do avoid them. It's a question about how noisy you want your robot to be. I'd prefer the robot to show up only when necessary. The coding style warnings are good for the developers who actively run checkpatch.pl to make their patch better. However most are probably not suitable for a robot to send people unsolicited warnings. SPACE_BEFORE_TAB TRAILING_SEMICOLON TRAILING_WHITESPACE USE_DEVICE_INITCALL USE_RELATIVE_PATH Having checkpatch tell people how to write changelogs I think not a great idea. These *ought* to make sense, but I don't know their false positive rates: HEXADECIMAL_BOOLEAN_TEST That's a good one. 0 false positives. ALLOC_ARRAY_ARGS Yes, this would be reasonable too. CONSIDER_KSTRTO I think orobably not. This would be a cleanup thing. Perhaps we can run it for a while, so that people at least come to aware there is a kstrto() for use. :) CONST_STRUCT OK SPLIT_STRING I suggest no but shrug Thanks for both of your suggestions! I'll add the commonly agreed ones: +INVALID_UTF8 +LINUX_VERSION_CODE +MISSING_EOF_NEWLINE +HEXADECIMAL_BOOLEAN_TEST +ALLOC_ARRAY_ARGS +CONST_STRUCT +CONSIDER_KSTRTO And remove the duplicate one (good catch, Josh!) -KREALLOC_ARG_REUSE Thanks, Fengguang -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Ksummit-2013-discuss] [PATCH] checkpatch: Add comment about updating Documentation/CodingStyle
On Tue, Sep 03, 2013 at 10:46:40AM +0800, Fengguang Wu wrote: On Mon, Sep 02, 2013 at 06:52:45PM -0700, Joe Perches wrote: On Mon, 2013-09-02 at 18:34 -0700, Josh Triplett wrote: CONFIG_EXPERIMENTAL CVS_KEYWORD OK, but shrug [...] Thanks for both of your suggestions! I'll add the commonly agreed ones: +INVALID_UTF8 +LINUX_VERSION_CODE +MISSING_EOF_NEWLINE +HEXADECIMAL_BOOLEAN_TEST +ALLOC_ARRAY_ARGS +CONST_STRUCT +CONSIDER_KSTRTO And remove the duplicate one (good catch, Josh!) -KREALLOC_ARG_REUSE You missed CONFIG_EXPERIMENTAL and CVS_KEYWORD; see above. - Josh Triplett -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Ksummit-2013-discuss] [PATCH] checkpatch: Add comment about updating Documentation/CodingStyle
On Mon, Sep 02, 2013 at 08:16:45PM -0700, Josh Triplett wrote: On Tue, Sep 03, 2013 at 10:46:40AM +0800, Fengguang Wu wrote: On Mon, Sep 02, 2013 at 06:52:45PM -0700, Joe Perches wrote: On Mon, 2013-09-02 at 18:34 -0700, Josh Triplett wrote: CONFIG_EXPERIMENTAL CVS_KEYWORD OK, but shrug [...] Thanks for both of your suggestions! I'll add the commonly agreed ones: +INVALID_UTF8 +LINUX_VERSION_CODE +MISSING_EOF_NEWLINE +HEXADECIMAL_BOOLEAN_TEST +ALLOC_ARRAY_ARGS +CONST_STRUCT +CONSIDER_KSTRTO And remove the duplicate one (good catch, Josh!) -KREALLOC_ARG_REUSE You missed CONFIG_EXPERIMENTAL and CVS_KEYWORD; see above. OK, added, thanks! Cheers, Fengguang -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
