From: Roland Fischer Date: Fri, 3 Jul 2020 04:22:03 +0000 (-0400) Subject: doc: Add dev code-style X-Git-Tag: suricata-6.0.0-rc1~100 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=de7c7eeff0da3fae3d16137298367e420be98661;p=thirdparty%2Fsuricata.git doc: Add dev code-style --- diff --git a/doc/devguide/codebase/code-style.rst b/doc/devguide/codebase/code-style.rst index 0c55cc99d8..5313391a07 100644 --- a/doc/devguide/codebase/code-style.rst +++ b/doc/devguide/codebase/code-style.rst @@ -6,12 +6,191 @@ Suricata uses a fairly strict coding style. This document describes it. Formatting ~~~~~~~~~~ +clang-format +^^^^^^^^^^^^ +``clang-format`` is configured to help you with formatting C code. + +.. Argh, github does not support admonitions such as .. note:: + ++-------------------------------------------------------+ +| **Note** | +| | +| The ``.clang-format`` file requires clang 9 or newer. | ++-------------------------------------------------------+ + +Format your Changes +******************* +Before opening a pull request, please also try to ensure it is formatted +properly. We use ``clang-format`` for this, which has git integration through the +``git-clang-format`` script to only format your changes. +On some systems, it may already be installed (or be installable via your package +manager). If so, you can simply run it. + +It is recommended to format each commit as you go. However, you can always +reformat your whole branch after the fact. + +.. Argh, github does not support admonitions such as .. note:: + ++----------------------------------------------------------------------------+ +| **Note** | +| | +| Depending on your installation, you might have to use the version-specific | +| ``git clang-format`` in the commands below, e.g. ``git clang-format-9``, | +| and possibly even provide the ``clang-format`` binary with | +| ``--binary clang-format-9``. | +| | +| As an alternative, you can use the provided ``scripts/clang-format.sh`` | +| that isolates you from the different versions. | ++----------------------------------------------------------------------------+ + +Formatting the most recent commit only +"""""""""""""""""""""""""""""""""""""" +The following command will format only the code changed in the most recent commit: + +.. code-block:: bash + + $ git clang-format HEAD^ + # Or with script: + $ scripts/clang-format.sh commit + +Note that this modifies the files, but doesn’t commit them – you’ll likely want to run + +.. code-block:: bash + + $ git commit --amend -a + +in order to update the last commit with all pending changes. + +Formatting code in staging +"""""""""""""""""""""""""" +The following command will format the changes in staging, i.e. files you ``git add``-ed: + +.. code-block:: bash + + $ git clang-format + # Or with script: + $ scripts/clang-format.sh cached + +If you also want to change the unstaged changes, do: + +.. code-block:: bash + + $ git clang-format --force + # Or with script: + $ scripts/clang-format.sh cached --force + +Formatting your branch' commits +""""""""""""""""""""""""""""""" +In case you have multiple commits on your branch already and forgot to +format them you can fix that up as well. + +The following command will format every commit in your branch off master and +rewrite history using the existing commit metadata. + +Tip: Create a new version of your branch first and run this off the new version. + +.. code-block:: bash + + # In a new version of your pull request: + $ scripts/clang-format.sh rewrite-branch + +You could also add the formatting as an additional commit "at the end". However, +this is frowned upon. It's preferred to use ``rewrite-branch`` instead. + +.. code-block:: bash + + # It's preferred to use rewrite-branch instead of this: + $ git clang-format first_commit_on_your_branch^ + # Or with script: + $ scripts/clang-format.sh branch + +Note the usage of ``first_commit_on_your_branch^``, not ``master``, to avoid picking up +new commits on master in case you've updated master since you've branched. + +Check formatting +"""""""""""""""" +Check if your branch changes' formatting is correct with: + +.. code-block:: bash + + $ scripts/clang-format.sh check-branch + +Add the ``--diffstat`` parameter if you want to see the files needing formatting. +Add the ``--diff`` parameter if you want to see the actual diff of the formatting +change. + +Formatting a whole file +""""""""""""""""""""""" + +.. Argh, github does not support admonitions such as .. note:: + ++--------------------------------------------------------------------+ +| **Note** | +| | +| Do not reformat whole files by default, i.e. do not use | +| ``clang-format`` proper in general. | ++--------------------------------------------------------------------+ + +If you were ever to do so, formatting changes of existing code with clang-format +shall be a different commit and must not be mixed with actual code changes. + +.. code-block:: bash + + $ clang-format -i {file} + +Disabling clang-format +********************** + +There might be times, where the clang-format's formatting might not please. +This might mostly happen with macros, arrays (single or multi-dimensional ones), +struct initialization, or where one manually formatted code. + +You can always disable clang-format. + +.. code-block:: c + + /* clang-format off */ + #define APP_LAYER_INCOMPLETE(c, n) (AppLayerResult){1, (c), (n)} + /* clang-format on */ + +Installing clang-format and git-clang-format +******************************************** +clang-format 9 or newer is required. + +On ubuntu 18.04: + +- It is sufficient to only install clang-format, e.g. + + .. code-block:: bash + + $ sudo apt-get install clang-format-9 + +- See http://apt.llvm.org for other releases in case the clang-format version + is not found in the default repos. + +On fedora: + +- Install the ``clang`` and ``git-clang-format`` packages with + + .. code-block:: bash + + $ sudo dnf install clang git-clang-format + + Line length ^^^^^^^^^^^ -There is a soft limit of ~80 characters. +Limit line lengths to 100 characters. + +When wrapping lines that are too long, they should be indented at least 8 +spaces from previous line. You should attempt to wrap the minimal portion of +the line to meet the 100 character limit. + +clang-format: + - ColumnLimit: 100 + - ContinuationIndentWidth: 8 + - ReflowComments: true -When wrapping lines that are too long, they should be indented at least 8 spaces from previous line. You should attempt to wrap the minimal portion of the line to meet the 80 character limit. Indent ^^^^^^ @@ -21,7 +200,7 @@ We use 4 space indentation. .. code-block:: c int DecodeEthernet(ThreadVars *tv, DecodeThreadVars *dtv, Packet *p, - uint8_t *pkt, uint16_t len, PacketQueue *pq) + uint8_t *pkt, uint16_t len, PacketQueue *pq) { SCPerfCounterIncr(dtv->counter_eth, tv->sc_perf_pca); @@ -30,6 +209,35 @@ We use 4 space indentation. return TM_ECODE_FAILED; } + ... + + DecodeNetworkLayer(tv, dtv, SCNtohs(p->ethh->eth_type), p, + pkt + ETHERNET_HEADER_LEN, len - ETHERNET_HEADER_LEN); + + return TM_ECODE_OK; + } + +Use 8 space indentation when wrapping function parameters, loops and if statements. + +Use 4 space indentation when wrapping variable definitions. + +.. code-block:: c + + const SCPlugin PluginSpec = { + .name = OUTPUT_NAME, + .author = "Some Developer", + .license = "GPLv2", + .Init = TemplateInit, + }; + + +clang-format: + - AlignAfterOpenBracket: DontAlign + - Cpp11BracedListStyle: false + - IndentWidth: 4 + - TabWidth: 8 [llvm]_ + - UseTab: Never [llvm]_ + Braces ^^^^^^ @@ -42,8 +250,9 @@ Functions should have the opening brace on a newline: DoSomething(); } +Note: this is a fairly new requirement, so you'll encounter a lot of non-compliant code. -Control statements should have the opening brace on the same line: +Control and loop statements should have the opening brace on the same line: .. code-block:: c @@ -52,6 +261,16 @@ Control statements should have the opening brace on the same line: return TM_ECODE_FAILED; } + for (ascii_code = 0; ascii_code < 256; ascii_code++) { + ctx->goto_table[ctx->state_count][ascii_code] = SC_AC_FAIL; + } + + while (funcs != NULL) { + temp = funcs; + funcs = funcs->next; + SCFree(temp); + } + Opening and closing braces go on the same line as as the _else_ (also known as a "cuddled else"). .. code-block:: c @@ -62,6 +281,41 @@ Opening and closing braces go on the same line as as the _else_ (also known as a DoThat(); } +Structs, unions and enums should have the opening brace on the same line: + +.. code-block:: c + + union { + TCPVars tcpvars; + ICMPV4Vars icmpv4vars; + ICMPV6Vars icmpv6vars; + } l4vars; + + struct { + uint8_t type; + uint8_t code; + } icmp_s; + + enum { + DETECT_TAG_TYPE_SESSION, + DETECT_TAG_TYPE_HOST, + DETECT_TAG_TYPE_MAX + }; + +clang-format: + - BreakBeforeBraces: Custom [breakbeforebraces]_ + - BraceWrapping: + + - AfterClass: true + - AfterControlStatement: false + - AfterEnum: false + - AfterFunction: true + - AfterStruct: false + - AfterUnion: false + - AfterExternBlock: true + - BeforeElse: false + - IndentBraces: false + Flow ~~~~ @@ -74,6 +328,24 @@ Don't use conditions and statements on the same line. E.g. if (a) b = a; // <- right + for (int i = 0; i < 32; ++i) f(i); // <- wrong + + for (int i = 0; i < 32; ++i) + f(i); // <- right + +Don't put short or empty functions and structs on one line. + +.. code-block:: c + + void empty_function(void) + { + } + + int short_function(void) + { + return 1; + } + Don't use unnecessary branching. E.g.: .. code-block:: c @@ -94,6 +366,61 @@ Can be written as: } a = b; +clang-format: + - AllowShortBlocksOnASingleLine: false [llvm]_ + - AllowShortBlocksOnASingleLine: Never [llvm]_ (breaking change in clang 10!) [clang10]_ + - AllowShortEnumsOnASingleLine: false [clang11]_ + - AllowShortFunctionsOnASingleLine: None + - AllowShortIfStatementsOnASingleLine: Never [llvm]_ + - AllowShortLoopsOnASingleLine: false [llvm]_ + - BreakBeforeBraces: Custom [breakbeforebraces]_ + - BraceWrapping: + + - SplitEmptyFunction: true + - SplitEmptyRecord: true + +Alignment +~~~~~~~~~ + +Pointers +^^^^^^^^ +Pointers shall be right aligned. + +.. code-block:: c + + void *ptr; + void f(int *a, const char *b); + void (*foo)(int *); + +clang-format: + - PointerAlignment: Right + - DerivePointerAlignment: false + +Declarations and Comments +^^^^^^^^^^^^^^^^^^^^^^^^^ +Trailing comments should be aligned for consecutive lines. + +.. code-block:: c + + struct bla { + int a; /* comment */ + unsigned bb; /* comment */ + int *ccc; /* comment */ + }; + + void alignment() + { + // multiple consecutive vars + int a = 13; /* comment */ + int32_t abc = 1312; /* comment */ + int abcdefghikl = 13; /* comment */ + } + +clang-format: + - AlignConsecutiveAssignments: false + - AlignConsecutiveDeclarations: false + - AlignTrailingComments: true + Functions ~~~~~~~~~ @@ -121,20 +448,6 @@ inline The inlining of functions should be used only in critical paths. -curly braces / brackets -^^^^^^^^^^^^^^^^^^^^^^^ - -Functions should have the opening bracket on a newline: - -.. code-block:: c - - int SomeFunction(void) - { - DoSomething(); - } - -Note: this is a fairly new requirement, so you'll encounter a lot of non-compliant code. - Variables ~~~~~~~~~ @@ -159,7 +472,34 @@ TODO Macros ~~~~~~ -TODO +Macro names are ALL_CAPS_WITH_UNDERSCORES. +Enclose parameters in parens on each usage inside the macro. + +Align macro values on consecutive lines. + +.. code-block:: c + + #define ACTION_ALERT 0x01 + #define ACTION_DROP 0x02 + #define ACTION_REJECT 0x04 + #define ACTION_REJECT_DST 0x08 + #define ACTION_REJECT_BOTH 0x10 + #define ACTION_PASS 0x20 + +Align escape for multi-line macros right-most at ColumnLimit. + +.. code-block:: c + + #define MULTILINE_DEF(a, b) \ + if ((a) > 2) { \ + auto temp = (b) / 2; \ + (b) += 10; \ + someFunctionCall((a), (b)); \ + } + +clang-format: + - AlignConsecutiveMacros: true [clang9]_ + - AlignEscapedNewlines: Right Comments ~~~~~~~~ @@ -205,7 +545,24 @@ Some cases have a multi-layer prefix, e.g. ``util-mpm-ac.c`` Enums ~~~~~ -TODO +Use a common prefix for all enum values. Value names are ALL_CAPS_WITH_UNDERSCORES. + +Put each enum values on a separate line. +Tip: Add a trailing comma to the last element to force "one-value-per-line" +formatting in clang-format. + +.. code-block:: c + + enum { VALUE_ONE, VALUE_TWO }; // <- wrong + + // right + enum { + VALUE_ONE, + VALUE_TWO, // <- force one-value-per-line + }; + +clang-format: + - AllowShortEnumsOnASingleLine: false [clang11]_ Structures and typedefs ~~~~~~~~~~~~~~~~~~~~~~~ @@ -241,6 +598,34 @@ Fall through cases will be commented with ``/* fall through */``. E.g.: default: default_packet_size = DEFAULT_PACKET_SIZE; + +Do not put short case labels on one line. +Put opening brace on same line as case statement. + +.. code-block:: c + + switch (a) { + case 13: { + int a = bla(); + break; + } + case 15: + blu(); + break; + default: + gugus(); + } + + +clang-format: + - IndentCaseLabels: true + - IndentCaseBlocks: false [clang11]_ + - AllowShortCaseLabelsOnASingleLine: false [llvm]_ + - BreakBeforeBraces: Custom [breakbeforebraces]_ + - BraceWrapping: + + - AfterCaseLabel: false (default) + const ~~~~~ @@ -275,10 +660,38 @@ Goto statements should be used with care. Generally, we use it primarily for err return NULL; } +Put goto labels at brace level. + +.. code-block:: c + + int goto_style_nested() + { + if (foo()) { + label1: + bar(); + } + + label2: + return 1; + } + +clang-format: + - IndentGotoLabels: true (default) [clang10]_ + +Includes +~~~~~~~~ + +TODO + +A .c file shall include it's own header first. + +clang-format: + - SortIncludes: false + Unittests ~~~~~~~~~ -When writing unittests that use when using a data array containing a protocol message, please put an explanatory comment that contain the readable content of the message +When writing unittests that use a data array containing a protocol message, please put an explanatory comment that contain the readable content of the message So instead of: @@ -332,3 +745,11 @@ Banned functions Also, check the existing code. If yours is wildly different, it's wrong. Example: https://github.com/oisf/suricata/blob/master/src/decode-ethernet.c + +.. rubric:: Footnotes + +.. [llvm] Default LLVM clang-format Style +.. [clang9] Requires clang 9 +.. [clang10] Requires clang 10 +.. [clang11] Requires clang 11 +.. [breakbeforebraces] BreakBeforeBraces: Mozilla is closest, but does not split empty functions/structs \ No newline at end of file