Newer
Older
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
124
125
126
127
128
129
130
131
# RIOT Maintainer Guidelines
This list presents a series of guidelines for maintainers, including technical
and non-technical. The list is not exhaustive, and represents a baseline on
things that should be ensured for contributions.
Notes:
- Any of the items in this list might be skipped, if clearly and logically
articulated reasons are presented.
- The order of the steps is meant to maximize efficiency and minimize
overhead, with the philosophy that failing in step n makes steps n+x
obsolete. However, this efficiency can depend on the quality of the
submission itself. If the PR (Pull Request) is clearly not in a reviewable
state (for example, due to poor code cleanliness or overall design), it
might be more efficient to give the contributor some broader comments for
improvement before further review.
- This is a working document and any changes, additions, or other discussions
are encouraged, via PRs raised against the document.
The [list of maintainers] gives information on the current maintainers and the
areas of RIOT they each maintain.
## Technical guidelines
### 1. - Review the fundamentals
Before spending the time on an in-depth code review, it's important to assess
the overall validity of the PR.
1. Does the reasoning for this PR make sense? \
Are the requirements and design goals clearly thought out and clearly
expressed? \
Is the problem that the PR intends to solve clearly stated?
2. Is the solution presented in the PR as simple as possible to satisfy the
requirements, but no simpler?
3. Is the solution well designed on a high level?
4. Do the concepts used by the PR make sense?
5. Does the PR break with existing concepts?
6. Is the structure of the PR itself valid?
7. Are there clear and adequate instructions on how to test the PR? \
This may or may not include implemented tests as part of the PR.
8. Does the code compile and run?
9. Does this PR respect the rights of previous authors, either through
retaining their commits or by retaining their copyrights in the boilerplate
headers?
10. Is the PR a duplicate of another PR?
### 2. - Review the design of the code
The following list is not exhaustive and addresses the coding issues we have
regularly seen in the past. In particular, check that the [Best Practices]
are followed. These checks can be aided (but not replaced) by a tool such as
Coccinelle, using the script found in dist/tools/coccinelle.
1. Check for code duplication
2. [Check memory usage][Comparing build sizes]
3. Check all code paths
4. Check for API compliance
5. Check for consistent error handling
6. Check scope of variables and functions
7. Check for syntactical, semantical or logical errors
8. Check for any runtime efficiency improvements that can be made in specific
lines of code - i.e., without changing the overall design of the code
9. Check that the code design is as simple as possible to solve the problem,
but no simpler
### 3. - Test the PR
Run tests to verify the correct behavior (see 1.6), both on `native` and on a
few selected boards, or present clearly and logically articulated reasons for
skipping some/all tests.
### 4. - Review the code against the coding conventions
Check that the code follows the [Coding Conventions]. This can be aided (but not
replaced) by Uncrustify, using the uncrustify-riot.cfg file found in the base
directory. Note the difference between personal coding style, which is
subjective and is allowed subject to the other guidelines, and the coding
conventions, which are absolute and must always be followed.
### 5. - Review the documentation
The aim of the documentation is to ensure that the code can be picked up as
easily as possible in the future. Ideally, the documentation is sufficiently
complete that no input from the original developer or maintainer is required.
1. Check for sufficient high-level (module-level) documentation
2. Verify function level documentation
3. Are critical/hard to understand parts in the code documented?
4. Check grammar and spelling of documentation
## Non-technical guidelines
### Interaction with contributors
- Be responsive. Even if you are too busy to review the contribution, try to
add a note fairly soon after the PR is submitted, thanking them for their
awesome contribution and saying that you will review it in due course. Once
the contributor has made their changes, ensure you reply to them in a
reasonable timeframe. Acknowledge their replies to concerns if you're happy
with their argument.
- Be helpful. Give precise and correct advice when possible and when it will
help the contributor. This can include code snippets, links to
code/issues/wiki entries/web pages, or anything else. Educating contributors
means we are investing in our community.
- Be friendly. Be objective in your review, and of course, follow the
[Code of Conduct].
### Organisation of reviewing between maintainers
This section is a placeholder for further discussion around making the review
process more efficient and effective when multiple maintainers are involved.
This could include:
- Usage of labels
- Division of review responsibilities, e.g. with ACKs for different areas of
review
- Usage of GitHub functionality, for example "Reviewers" and "Assignees" lists
[list of maintainers]: https://github.com/RIOT-OS/RIOT/wiki/Maintainers
[Best Practices]: https://github.com/RIOT-OS/RIOT/wiki/Best-Practice-for-RIOT-Programming
[Comparing build sizes]: https://github.com/RIOT-OS/RIOT/wiki/Comparing-build-sizes
[Coding Conventions]: https://github.com/RIOT-OS/RIOT/wiki/Coding-conventions
[Code of Conduct]: https://github.com/RIOT-OS/RIOT/blob/master/CODE_OF_CONDUCT.md