more thoughts
[asterisk/asterisk.git] / doc / CODING-GUIDELINES
1 == Asterisk Patch/Coding Guidelines ==
2
3 To be accepted into the codebase, all non-trivial changes must be
4 disclaimed to Digium or placed in the public domain. For more information
5 see http://bugs.digium.com
6
7 Patches should be in the form of a unified (-u) diff, made from the directory
8 above the top-level Asterisk source directory. For example:
9
10 - the base code you are working from is in ~/work/asterisk-base
11 - the changes are in ~/work/asterisk-new
12
13 ~/work$ diff -urN asterisk-base asterisk-new
14
15 All code, filenames, function names and comments must be in ENGLISH.
16
17 Do not declare variables mid-function (e.g. like recent  GNU compilers support)
18 since it is harder to read and not portable to GCC 2.95 and others.
19
20 Don't annotate your changes with comments like "/* JMG 4/20/04 */";
21 Comments should explain what the code does, not when something was changed
22 or who changed it.
23
24 Don't make unnecessary whitespace changes throughout the code.
25
26 Don't use C++ type (//) comments.
27
28 Try to match the existing formatting of the file you are working on.
29
30 Functions and variables that are not intended to be used outside the module
31 must be declared static.
32
33 When reading integer numeric input with scanf (or variants), do _NOT_ use '%i'
34 unless you specifically want to allow non-base-10 input; '%d' is always a better
35 choice, since it will not silently turn numbers with leading zeros into base-8.
36
37 Roughly, Asterisk code formatting guidelines are generally equivalent to the 
38 following:
39
40 # indent -i4 -ts4 -br -brs -cdw -cli0 -ce -nbfda -npcs -npsl foo.c
41
42 Function calls and arguments should be spaced in a consistent way across
43 the codebase.
44 GOOD: foo(arg1, arg2);
45 GOOD: foo(arg1,arg2);   /* Acceptable but not preferred */
46 BAD: foo (arg1, arg2);
47 BAD: foo( arg1, arg2 );
48 BAD: foo(arg1, arg2,arg3);
49
50 Don't treat keywords (if, while, do, return) as if they were functions;
51 leave space between the keyword and the expression used (if any). For 'return',
52 don't even put parentheses around the expression, since they are not
53 required.
54
55 There is no shortage of whitespace characters :-) Use them when they make
56 the code easier to read. For example:
57
58 for (str=foo;str;str=str->next)
59
60 is harder to read than
61
62 for (str = foo; str; str = str->next)
63
64 Following are examples of how code should be formatted.
65
66 Functions:
67 int foo(int a, char *s)
68 {
69         return 0;
70 }
71
72 If statements:
73 if (foo) {
74         bar();
75 } else {
76         blah();
77 }
78
79 Case statements:
80 switch (foo) {
81 case BAR:
82         blah();
83         break;
84 case OTHER:
85         other();
86         break;
87 }
88
89 No nested statements without braces, e.g.:
90
91 for (x = 0; x < 5; x++)
92         if (foo) 
93                 if (bar)
94                         baz();
95
96 instead do:
97 for (x = 0; x < 5; x++) {
98         if (foo) {
99                 if (bar)
100                         baz();
101         }
102 }
103
104 Don't build code like this:
105
106 if (foo) {
107         /* .... 50 lines of code ... */
108 } else {
109         result = 0;
110         return;
111 }
112
113 Instead, try to minimize the number of lines of code that need to be
114 indented, by only indenting the shortest case of the 'if'
115 statement, like so:
116
117 if !(foo) {
118         result = 0;
119         return;
120 }
121
122 .... 50 lines of code ....
123
124 When this technique is used properly, it makes functions much easier to read
125 and follow, especially those with more than one or two 'setup' operations
126 that must succeed for the rest of the function to be able to execute.
127
128 Proper use of this technique may occasionally result in the need for a
129 label/goto combination so that error/failure conditions can exit the
130 function while still performing proper cleanup. This is not a bad thing!
131 Use of goto in this situation is encouraged, since it removes the need
132 for excess code indenting without requiring duplication of cleanup code.
133        
134 Make sure you never use an uninitialized variable.  The compiler will 
135 usually warn you if you do so. However, do not go too far the other way,
136 and needlessly initialize variables that do not require it. If the first
137 time you use a variable in a function is to store a value there, then
138 initializing it at declaration is pointless, and will generate extra
139 object code and data in the resulting binary with no purpose. When in doubt,
140 trust the compiler to tell you when you need to initialize a variable;
141 if it does not warn you, initialization is not needed.
142
143 Do not explicitly cast 'void *' into any other type, nor should you cast any
144 other type into 'void *'. Implicit casts to/from 'void *' are explicitly
145 allowed by the C specification. This means the results of malloc(), calloc(),
146 alloca(), and similar functions do not _ever_ need to be cast to a specific
147 type, and when you are passing a pointer to (for example) a callback function
148 that accepts a 'void *' you do not need to cast into that type.
149
150 Name global variables (or local variables when you have a lot of them or
151 are in a long function) something that will make sense to aliens who
152 find your code in 100 years.  All variable names should be in lower 
153 case, except when following external APIs or specifications that normally
154 use upper- or mixed-case variable names; in that situation, it is
155 preferable to follow the external API/specification for ease of
156 understanding.
157
158 Make some indication in the name of global variables which represent
159 options that they are in fact intended to be global.
160  e.g.: static char global_something[80]
161
162 Don't use 'typedef' just to shorten the amount of typing; there is no substantial
163 benefit in this:
164
165 struct foo {
166         int bar;
167 };
168 typedef foo_t struct foo;
169
170 In fact, don't use 'variable type' suffixes at all; it's much preferable to
171 just type 'struct foo' rather than 'foo_s'.
172
173 Use enums rather than long lists of #define-d numeric constants when possible;
174 this allows structure members, local variables and function arguments to
175 be declared as using the enum's type. For example:
176
177 enum option {
178   OPT_FOO = 1
179   OPT_BAR = 2
180   OPT_BAZ = 4
181 };
182
183 static enum option global_option;
184
185 static handle_option(const enum option opt)
186 {
187   ...
188 }
189
190 Note: The compiler will _not_ force you to pass an entry from the enum
191 as an argument to this function; this recommendation serves only to make
192 the code clearer and somewhat self-documenting.
193
194 When making applications, always ast_strdupa(data) to a local pointer if
195 you intend to parse the incoming data string.
196
197         if (data)
198                 mydata = ast_strdupa(data);
199
200 Use ast_separate_app_args() to separate the arguments to your application
201 once you have made a local copy of the string.
202
203 Use strsep() for parsing strings when possible; there is no worry about
204 're-entrancy' as with strtok(), and even though it modifies the original
205 string (which the man page warns about), in many cases that is exactly
206 what you want!
207
208 Always derefrence or localize pointers to things that are not yours like
209 channel members in a channel that is not associated with the current 
210 thread and for which you do not have a lock.
211         channame = ast_strdupa(otherchan->name);
212
213 If you do the same or a similar operation more than 1 time, make it a
214 function or macro.
215
216 Make sure you are not duplicating any functionality already found in an
217 API call somewhere.  If you are duplicating functionality found in 
218 another static function, consider the value of creating a new API call 
219 which can be shared.
220
221 As a common example of this point, make an effort to use the lockable
222 linked-list macros found in include/asterisk/linkedlists.h. They are
223 efficient, easy to use and provide every operation that should be
224 necessary for managing a singly-linked list (if something is missing,
225 let us know!). Just because you see other open-coded list implementations
226 in the source tree is no reason to continue making new copies of
227 that code... There are also a number of common string manipulation
228 and timeval manipulation functions in asterisk/strings.h and asterisk/time.h;
229 use them when possible.
230
231 When you achieve your desired functionalty, make another few refactor
232 passes over the code to optimize it.
233
234 Before submitting a patch, *read* the actual patch file to be sure that 
235 all the changes you expect to be there are, and that there are no 
236 surprising changes you did not expect.
237
238 If you are asked to make changes to your patch, there is a good chance
239 the changes will introduce bugs, check it even more at this stage.
240
241 Avoid needless malloc(), strdup() calls. If you only need the value in
242 the scope of your function try ast_strdupa() or declare structs on the
243 stack and pass a pointer to them. However, be careful to _never_ call
244 alloca(), ast_strdupa() or similar functions in the argument list
245 of a function you are calling; this can cause very strange stack
246 arrangements and produce unexpected behavior.
247
248 If you are going to reuse a computed value, save it in a variable
249 instead of recomputing it over and over.  This can prevent you from 
250 making a mistake in subsequent computations, make it easier to correct
251 if the formula has an error and may or may not help optimization but 
252 will at least help readability.
253
254 Just an example, so don't over analyze it, that'd be a shame:
255
256 const char *prefix = "pre";     
257 const char *postfix = "post";
258 char *newname;
259 char *name = "data";
260
261 if (name && (newname = alloca(strlen(name) + strlen(prefix) + strlen(postfix) + 3)))
262         snprintf(newname, strlen(name) + strlen(prefix) + strlen(postfix) + 3, "%s/%s/%s", prefix, name, postfix);
263
264 vs
265
266 const char *prefix = "pre";
267 const char *postfix = "post";
268 char *newname;
269 char *name = "data";
270 int len = 0;
271
272 if (name && (len = strlen(name) + strlen(prefix) + strlen(postfix) + 3) && (newname = alloca(len)))
273         snprintf(newname, len, "%s/%s/%s", prefix, name, postfix);
274
275
276 Use const on pointer arguments which your function will not be modifying, as this 
277 allows the compiler to make certain optimizations. In general, use 'const'
278 on any argument that you have no direct intention of modifying, as it can
279 catch logic/typing errors in your code when you use the argument variable
280 in a way that you did not intend.
281
282 Don't use strncpy for copying whole strings; it does not guarantee that the
283 output buffer will be null-terminated. Use ast_copy_string instead, which
284 is also slightly more efficient (and allows passing the actual buffer
285 size, which makes the code clearer).
286
287 Don't use ast_copy_string (or any length-limited copy function) for copying
288 fixed (known at compile time) strings into buffers, if the buffer is something
289 that has been allocated in the function doing the copying. In that case, you
290 know at the time you are writing the code whether the buffer is large enough
291 for the fixed string or not, and if it's not, your code won't work anyway!
292 Use strcpy() for this operation, or directly set the first two characters
293 of the buffer if you are just trying store a one-character string in the
294 buffer. If you are trying to 'empty' the buffer, just store a single
295 NULL character ('\0') in the first byte of the buffer; nothing else is
296 needed, and any other method is wasteful.
297
298 In addition, if the previous operations in the function have already
299 determined that the buffer in use is adequately sized to hold the string
300 you wish to put into it (even if you did not allocate the buffer yourself),
301 use a direct strcpy(), as it can be inlined and optimized to simple
302 processor operations, unlike ast_copy_string().
303
304 When allocating/zeroing memory for a structure, try to use code like this:
305
306 struct foo *tmp;
307
308 ...
309
310 tmp = malloc(sizeof(*tmp));
311 if (tmp)
312         memset(tmp, 0, sizeof(*tmp));
313
314 This eliminates duplication of the 'struct foo' identifier, which makes the
315 code easier to read and also ensures that if it is copy-and-pasted it won't
316 require as much editing. In fact, you can even use:
317
318 struct foo *tmp;
319
320 ...
321
322 tmp = calloc(1, sizeof(*tmp));
323
324 This will allocate and zero the memory in a single operation.
325
326 == CLI Commands ==
327
328 New CLI commands should be named using the module's name, followed by a verb
329 and then any parameters that the command needs. For example:
330
331 *CLI> iax2 show peer <peername>
332
333 not
334
335 *CLI> show iax2 peer <peername>
336
337 == New dialplan applications/functions ==
338
339 There are two methods of adding functionality to the Asterisk
340 dialplan: applications and functions. Applications (found generally in
341 the apps/ directory) should be collections of code that interact with
342 a channel and/or user in some significant way. Functions (which can be
343 provided by any type of module) are used when the provided
344 functionality is simple... getting/retrieving a value, for
345 example. Functions should also be used when the operation is in no way
346 related to a channel (a computation or string operation, for example).
347
348 Applications are registered and invoked using the
349 ast_register_application function; see the apps/app_skel.c file for an
350 example.
351
352 Functions are registered using 'struct ast_custom_function'
353 structures and the ast_custom_function_register function.
354
355 == Doxygen API Documentation Guidelines ==
356
357 When writing Asterisk API documentation the following format should be
358 followed.
359
360 /*!
361  * \brief Do interesting stuff.
362  * \param thing1 interesting parameter 1.
363  * \param thing2 interesting parameter 2.
364  *
365  * This function does some interesting stuff.
366  *
367  * \return zero on success, -1 on error.
368  */
369 int ast_interesting_stuff(int thing1, int thing2)
370 {
371         return 0;
372 }
373
374 Notice the use of the \param, \brief, and \return constructs.  These should be
375 used to describe the corresponding pieces of the function being documented.
376 Also notice the blank line after the last \param directive.  All doxygen
377 comments must be in one /*! */ block.  If the function or struct does not need
378 an extended description it can be left out.
379
380 Please make sure to review the doxygen manual and make liberal use of the \a,
381 \code, \c, \b, \note, \li and \e modifiers as appropriate.
382
383 When documenting a 'static' function or an internal structure in a module,
384 use the \internal modifier to ensure that the resulting documentation
385 explicitly says 'for internal use only'.
386
387 Structures should be documented as follows.
388
389 /*!
390  * \brief A very interesting structure.
391  */
392 struct interesting_struct
393 {
394         /*! \brief A data member. */
395         int member1;
396
397         int member2; /*!< \brief Another data member. */
398 }
399
400 Note that /*! */ blocks document the construct immediately following them
401 unless they are written, /*!< */, in which case they document the construct
402 preceding them.