docs/review_comments.md

   1 # Code Review Comments
   2
   3 This is a collection of frequent code review comments, collected here for
   4 reference and discussed in more depth than a typical code review would allow.
   5
   6 The intended use for this document is to point to it from a code review to make
   7 a point quickly while still providing the contributor with enough information
   8 to resolve the issue. For example, a good review comment would be:
   9
  10 ![Please initialize variables at declaration. Link to comment.](review_comments_example.png)
  11
  12 A link to each paragraph is provided at the beginning for easy copy'n'pasting.
  13
  14 ## Initialize variables
  15
  16 → [https://collectd.org/review-comments#initialize-variables](https://collectd.org/review-comments#initialize-variables)
  17
  18 Initialize variables when declaring them. By default, C does not initialize
  19 variables when they are declared. If a code path ends up reading the variable
  20 before it is initialized, for example because a loop body is never
  21 executed, it will read random data, causing undefined behavior. Worst case,
  22 pointers will point to random memory causing a segmentation fault.
  23
  24 **Examples:**
  25
  26 ```c
  27 /* Initialize scalar with to literal: */
  28 int status = 0;
  29
  30 /* Initialize pointer with function call: */
  31 char *buffer = calloc(1, buffer_size);
  32
  33 /* Initialize struct with struct initializer: */
  34 struct addrinfo ai = {
  35   .ai_family = AF_UNSPEC,
  36   .ai_flags = AI_ADDRCONFIG,
  37   .ai_socktype = SOCK_STREAM,
  38 };
  39
  40 /* Initialize struct with zero: */
  41 struct stat statbuf = {0};
  42 ```
  43
  44 In the last example, `{0}` is the universal struct initializer that, in theory,
  45 should be able to zero-initialize any struct. In practise, however, some
  46 compilers don't implement this correctly and will get confused when the first
  47 member is a struct or a union. Our *continuous integration* framework will
  48 catch these cases.
  49
  50 In many cases, this means declaring variables later. For example, this *bad*
  51 example:
  52
  53 ```c
  54 int status; /* BAD */
  55
  56 /* other code */
  57
  58 status = function_call();
  59 ```
  60
  61 Would become:
  62
  63 ```c
  64 /* other code */
  65
  66 int status = function_call(); /* GOOD */
  67 ```