]> git.ipfire.org Git - thirdparty/suricata.git/commitdiff
doc: Add dev code-style
authorRoland Fischer <roli@gugus.ca>
Fri, 3 Jul 2020 04:22:03 +0000 (00:22 -0400)
committerVictor Julien <victor@inliniac.net>
Mon, 31 Aug 2020 13:16:28 +0000 (15:16 +0200)
doc/devguide/codebase/code-style.rst

index 0c55cc99d8ef40509dd75d1e7059f3d1eb640441..5313391a073f0cd7f32143293b61a8701931c676 100644 (file)
@@ -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