diff options
author | Jonathan Corbet <corbet@lwn.net> | 2014-12-23 10:38:24 -0500 |
---|---|---|
committer | Jonathan Corbet <corbet@lwn.net> | 2014-12-23 10:38:24 -0500 |
commit | 6de16eba62b3b4d01b2b232ea7724d5450a19e30 (patch) | |
tree | 33782c7902de447cbd1b4f3a1abf47b322bfb2cc /Documentation/SubmittingPatches | |
parent | 97bf6af1f928216fd6c5a66e8a57bfa95a659672 (diff) |
Docs: Remove "tips and tricks" from SubmittingPatches
This section was just a weird collection of stuff that is better found
elsewhere. The "coding style" section somewhat duplicated the previous
coding style section; the useful information there has been collected into
a single place.
Signed-off-by: Jonathan Corbet <corbet@lwn.net>
Diffstat (limited to 'Documentation/SubmittingPatches')
-rw-r--r-- | Documentation/SubmittingPatches | 117 |
1 files changed, 21 insertions, 96 deletions
diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches index 1fa1caa198eb..8f416a2b409f 100644 --- a/Documentation/SubmittingPatches +++ b/Documentation/SubmittingPatches | |||
@@ -193,17 +193,33 @@ then only post say 15 or so at a time and wait for review and integration. | |||
193 | 193 | ||
194 | 194 | ||
195 | 195 | ||
196 | 4) Style check your changes. | 196 | 4) Style-check your changes. |
197 | ---------------------------- | ||
197 | 198 | ||
198 | Check your patch for basic style violations, details of which can be | 199 | Check your patch for basic style violations, details of which can be |
199 | found in Documentation/CodingStyle. Failure to do so simply wastes | 200 | found in Documentation/CodingStyle. Failure to do so simply wastes |
200 | the reviewers time and will get your patch rejected, probably | 201 | the reviewers time and will get your patch rejected, probably |
201 | without even being read. | 202 | without even being read. |
202 | 203 | ||
203 | At a minimum you should check your patches with the patch style | 204 | One significant exception is when moving code from one file to |
204 | checker prior to submission (scripts/checkpatch.pl). You should | 205 | another -- in this case you should not modify the moved code at all in |
205 | be able to justify all violations that remain in your patch. | 206 | the same patch which moves it. This clearly delineates the act of |
207 | moving the code and your changes. This greatly aids review of the | ||
208 | actual differences and allows tools to better track the history of | ||
209 | the code itself. | ||
210 | |||
211 | Check your patches with the patch style checker prior to submission | ||
212 | (scripts/checkpatch.pl). Note, though, that the style checker should be | ||
213 | viewed as a guide, not as a replacement for human judgment. If your code | ||
214 | looks better with a violation then its probably best left alone. | ||
206 | 215 | ||
216 | The checker reports at three levels: | ||
217 | - ERROR: things that are very likely to be wrong | ||
218 | - WARNING: things requiring careful review | ||
219 | - CHECK: things requiring thought | ||
220 | |||
221 | You should be able to justify all violations that remain in your | ||
222 | patch. | ||
207 | 223 | ||
208 | 224 | ||
209 | 5) Select e-mail destination. | 225 | 5) Select e-mail destination. |
@@ -684,100 +700,9 @@ new/deleted or renamed files. | |||
684 | With rename detection, the statistics are rather different [...] | 700 | With rename detection, the statistics are rather different [...] |
685 | because git will notice that a fair number of the changes are renames. | 701 | because git will notice that a fair number of the changes are renames. |
686 | 702 | ||
687 | ----------------------------------- | ||
688 | SECTION 2 - HINTS, TIPS, AND TRICKS | ||
689 | ----------------------------------- | ||
690 | |||
691 | This section lists many of the common "rules" associated with code | ||
692 | submitted to the kernel. There are always exceptions... but you must | ||
693 | have a really good reason for doing so. You could probably call this | ||
694 | section Linus Computer Science 101. | ||
695 | |||
696 | |||
697 | |||
698 | 1) Read Documentation/CodingStyle | ||
699 | |||
700 | Nuff said. If your code deviates too much from this, it is likely | ||
701 | to be rejected without further review, and without comment. | ||
702 | |||
703 | One significant exception is when moving code from one file to | ||
704 | another -- in this case you should not modify the moved code at all in | ||
705 | the same patch which moves it. This clearly delineates the act of | ||
706 | moving the code and your changes. This greatly aids review of the | ||
707 | actual differences and allows tools to better track the history of | ||
708 | the code itself. | ||
709 | |||
710 | Check your patches with the patch style checker prior to submission | ||
711 | (scripts/checkpatch.pl). The style checker should be viewed as | ||
712 | a guide not as the final word. If your code looks better with | ||
713 | a violation then its probably best left alone. | ||
714 | |||
715 | The checker reports at three levels: | ||
716 | - ERROR: things that are very likely to be wrong | ||
717 | - WARNING: things requiring careful review | ||
718 | - CHECK: things requiring thought | ||
719 | |||
720 | You should be able to justify all violations that remain in your | ||
721 | patch. | ||
722 | |||
723 | |||
724 | |||
725 | 2) #ifdefs are ugly | ||
726 | |||
727 | Code cluttered with ifdefs is difficult to read and maintain. Don't do | ||
728 | it. Instead, put your ifdefs in a header, and conditionally define | ||
729 | 'static inline' functions, or macros, which are used in the code. | ||
730 | Let the compiler optimize away the "no-op" case. | ||
731 | |||
732 | Simple example, of poor code: | ||
733 | |||
734 | dev = alloc_etherdev (sizeof(struct funky_private)); | ||
735 | if (!dev) | ||
736 | return -ENODEV; | ||
737 | #ifdef CONFIG_NET_FUNKINESS | ||
738 | init_funky_net(dev); | ||
739 | #endif | ||
740 | |||
741 | Cleaned-up example: | ||
742 | |||
743 | (in header) | ||
744 | #ifndef CONFIG_NET_FUNKINESS | ||
745 | static inline void init_funky_net (struct net_device *d) {} | ||
746 | #endif | ||
747 | |||
748 | (in the code itself) | ||
749 | dev = alloc_etherdev (sizeof(struct funky_private)); | ||
750 | if (!dev) | ||
751 | return -ENODEV; | ||
752 | init_funky_net(dev); | ||
753 | |||
754 | |||
755 | |||
756 | 3) 'static inline' is better than a macro | ||
757 | |||
758 | Static inline functions are greatly preferred over macros. | ||
759 | They provide type safety, have no length limitations, no formatting | ||
760 | limitations, and under gcc they are as cheap as macros. | ||
761 | |||
762 | Macros should only be used for cases where a static inline is clearly | ||
763 | suboptimal [there are a few, isolated cases of this in fast paths], | ||
764 | or where it is impossible to use a static inline function [such as | ||
765 | string-izing]. | ||
766 | |||
767 | 'static inline' is preferred over 'static __inline__', 'extern inline', | ||
768 | and 'extern __inline__'. | ||
769 | |||
770 | |||
771 | |||
772 | 4) Don't over-design. | ||
773 | |||
774 | Don't try to anticipate nebulous future cases which may or may not | ||
775 | be useful: "Make it as simple as you can, and no simpler." | ||
776 | |||
777 | |||
778 | 703 | ||
779 | ---------------------- | 704 | ---------------------- |
780 | SECTION 3 - REFERENCES | 705 | SECTION 2 - REFERENCES |
781 | ---------------------- | 706 | ---------------------- |
782 | 707 | ||
783 | Andrew Morton, "The perfect patch" (tpp). | 708 | Andrew Morton, "The perfect patch" (tpp). |