Fix a couple of warnings in test suite
[collectd.git] / 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 ```