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 local variables when they are defined. If a code path ends up reading the
  20 variable 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 ## Define variables on first use
  51
  52 → [https://collectd.org/review-comments#define-variables-on-first-use](https://collectd.org/review-comments#define-variables-on-first-use)
  53
  54 Local variables should be defined when they are first used, ideally when they
  55 can be initialized. For example:
  56
  57 ```c
  58 struct foo *f = calloc(1, sizeof(*f));
  59 if (f == NULL) {
  60   return ENOMEM;
  61 }
  62
  63 /* GOOD: status defiened and initialized late. */
  64 int status = function_call(f);
  65 ```
  66
  67 Sometimes variables are initialized by passing a pointer to them to a function.
  68 In that case define them as close to the function call as you can and
  69 zero-initialize them. The function may only partially initialize a struct or
  70 not initialize a struct at all in some circumstances.
  71
  72 **Example:**
  73
  74 ```c
  75 char const *path = determine_path();
  76
  77 struct stat s = {0};
  78 int status = stat(path, &s);
  79 ```
  80
  81 Old C standards (C89 and ealier) required variables to be defined at the
  82 beginning of a scope block. The following *bad* style is still frequently
  83 found:
  84
  85 ```c
  86 /* BAD: local variables defined at beginning of block. */
  87 struct foo *f;
  88 int status;
  89
  90 f = calloc(1, sizeof(*f));
  91 if (f == NULL) {
  92   return ENOMEM;
  93 }
  94
  95 status = function_call(f);
  96 ```