Various cleanups from comments in an email from Luigi Rizzo. Thank you!
[asterisk/asterisk.git] / doc / CODING-GUIDELINES
1 == Asterisk Patch/Coding Guidelines ==
2 --------------------------------------
3
4 We are looking forward to your contributions to Asterisk - the 
5 Open Source PBX! As Asterisk is a large and in some parts very
6 time-sensitive application, the code base needs to conform to
7 a common set of coding rules so that many developers can enhance
8 and maintain the code. Code also needs to be reviewed and tested
9 so that it works and follows the general architecture and guide-
10 lines, and is well documented.
11
12 Asterisk is published under a dual-licensing scheme by Digium.
13 To be accepted into the codebase, all non-trivial changes must be
14 disclaimed to Digium or placed in the public domain. For more information
15 see http://bugs.digium.com
16
17 Patches should be in the form of a unified (-u) diff, made from the directory
18 above the top-level Asterisk source directory. For example:
19
20 - the base code you are working from is in ~/work/asterisk-base
21 - the changes are in ~/work/asterisk-new
22
23 ~/work$ diff -urN asterisk-base asterisk-new
24
25 If you are changing within a fresh CVS/SVN repository, you can create
26 a patch with
27 $ cvs diff -urN <mycodefile>.c
28
29 * General rules
30 ---------------
31
32 - All code, filenames, function names and comments must be in ENGLISH.
33
34 - Don't annotate your changes with comments like "/* JMG 4/20/04 */";
35   Comments should explain what the code does, not when something was changed
36   or who changed it. If you have done a larger contribution, make sure
37   that you are added to the CREDITS file.
38
39 - Don't make unnecessary whitespace changes throughout the code.
40   If you make changes, submit them to the tracker as separate patches
41   that only include whitespace and formatting changes.
42
43 - Don't use C++ type (//) comments.
44
45 - Try to match the existing formatting of the file you are working on.
46
47 - Use spaces instead of tabs when aligning in-line comments or #defines (this makes 
48   your comments aligned even if the code is viewed with another tabsize)
49
50 * Declaration of functions and variables
51 ----------------------------------------
52
53 - Do not declare variables mid-block (e.g. like recent GNU compilers support)
54   since it is harder to read and not portable to GCC 2.95 and others.
55
56 - Functions and variables that are not intended to be used outside the module
57   must be declared static.
58
59 - When reading integer numeric input with scanf (or variants), do _NOT_ use '%i'
60   unless you specifically want to allow non-base-10 input; '%d' is always a better
61   choice, since it will not silently turn numbers with leading zeros into base-8.
62
63 - Strings that are coming from input should not be used as a first argument to
64   a formatted *printf function.
65
66 * Use the internal API
67 ----------------------
68
69 - Make sure you are aware of the string and data handling functions that exist
70   within Asterisk to enhance portability and in some cases to produce more
71   secure and thread-safe code. Check utils.c/utils.h for these.
72
73
74 * Code formatting
75 -----------------
76
77 Roughly, Asterisk code formatting guidelines are generally equivalent to the 
78 following:
79
80 # indent -i4 -ts4 -br -brs -cdw -cli0 -ce -nbfda -npcs -nprs -npsl -saf -sai -saw foo.c
81
82 this means in verbose:
83  -i4:    indent level 4
84  -ts4:   tab size 4
85  -br:    braces on if line
86  -brs:   braces on struct decl line
87  -cdw:   cuddle do while
88  -cli0:  case indentation 0
89  -ce:    cuddle else
90  -nbfda: dont break function decl args
91  -npcs:  no space after function call names
92  -nprs:  no space after parentheses
93  -npsl:  dont break procedure type
94  -saf:   space after for
95  -sai:   space after if
96  -saw:   space after while
97
98 Function calls and arguments should be spaced in a consistent way across
99 the codebase.
100         GOOD: foo(arg1, arg2);
101         GOOD: foo(arg1,arg2);   /* Acceptable but not preferred */
102         BAD: foo (arg1, arg2);
103         BAD: foo( arg1, arg2 );
104         BAD: foo(arg1, arg2,arg3);
105
106 Don't treat keywords (if, while, do, return) as if they were functions;
107 leave space between the keyword and the expression used (if any). For 'return',
108 don't even put parentheses around the expression, since they are not
109 required.
110
111 There is no shortage of whitespace characters :-) Use them when they make
112 the code easier to read. For example:
113
114         for (str=foo;str;str=str->next)
115
116 is harder to read than
117
118         for (str = foo; str; str = str->next)
119
120 Following are examples of how code should be formatted.
121
122 - Functions:
123 int foo(int a, char *s)
124 {
125         return 0;
126 }
127
128 - If statements:
129 if (foo) {
130         bar();
131 } else {
132         blah();
133 }
134
135 - Case statements:
136 switch (foo) {
137 case BAR:
138         blah();
139         break;
140 case OTHER:
141         other();
142         break;
143 }
144
145 - No nested statements without braces, e.g.:
146
147 for (x = 0; x < 5; x++)
148         if (foo) 
149                 if (bar)
150                         baz();
151
152 instead do:
153 for (x = 0; x < 5; x++) {
154         if (foo) {
155                 if (bar)
156                         baz();
157         }
158 }
159
160 - Don't build code like this:
161
162 if (foo) {
163         /* .... 50 lines of code ... */
164 } else {
165         result = 0;
166         return;
167 }
168
169 Instead, try to minimize the number of lines of code that need to be
170 indented, by only indenting the shortest case of the 'if'
171 statement, like so:
172
173 if !(foo) {
174         result = 0;
175         return;
176 }
177
178 .... 50 lines of code ....
179
180 When this technique is used properly, it makes functions much easier to read
181 and follow, especially those with more than one or two 'setup' operations
182 that must succeed for the rest of the function to be able to execute.
183
184 - Labels/goto are acceptable
185 Proper use of this technique may occasionally result in the need for a
186 label/goto combination so that error/failure conditions can exit the
187 function while still performing proper cleanup. This is not a bad thing!
188 Use of goto in this situation is encouraged, since it removes the need
189 for excess code indenting without requiring duplication of cleanup code.
190        
191 - Never use an uninitialized variable
192 Make sure you never use an uninitialized variable.  The compiler will 
193 usually warn you if you do so. However, do not go too far the other way,
194 and needlessly initialize variables that do not require it. If the first
195 time you use a variable in a function is to store a value there, then
196 initializing it at declaration is pointless, and will generate extra
197 object code and data in the resulting binary with no purpose. When in doubt,
198 trust the compiler to tell you when you need to initialize a variable;
199 if it does not warn you, initialization is not needed.
200
201 - Do not cast 'void *'
202 Do not explicitly cast 'void *' into any other type, nor should you cast any
203 other type into 'void *'. Implicit casts to/from 'void *' are explicitly
204 allowed by the C specification. This means the results of malloc(), calloc(),
205 alloca(), and similar functions do not _ever_ need to be cast to a specific
206 type, and when you are passing a pointer to (for example) a callback function
207 that accepts a 'void *' you do not need to cast into that type.
208
209 * Variable naming
210 -----------------
211
212 - Global variables
213 Name global variables (or local variables when you have a lot of them or
214 are in a long function) something that will make sense to aliens who
215 find your code in 100 years.  All variable names should be in lower 
216 case, except when following external APIs or specifications that normally
217 use upper- or mixed-case variable names; in that situation, it is
218 preferable to follow the external API/specification for ease of
219 understanding.
220
221 Make some indication in the name of global variables which represent
222 options that they are in fact intended to be global.
223  e.g.: static char global_something[80]
224
225 - Don't use un-necessary typedef's
226 Don't use 'typedef' just to shorten the amount of typing; there is no substantial
227 benefit in this:
228
229 struct foo {
230         int bar;
231 };
232 typedef foo_t struct foo;
233
234 In fact, don't use 'variable type' suffixes at all; it's much preferable to
235 just type 'struct foo' rather than 'foo_s'.
236
237 - Use enums instead of #define where possible
238 Use enums rather than long lists of #define-d numeric constants when possible;
239 this allows structure members, local variables and function arguments to
240 be declared as using the enum's type. For example:
241
242 enum option {
243   OPT_FOO = 1
244   OPT_BAR = 2
245   OPT_BAZ = 4
246 };
247
248 static enum option global_option;
249
250 static handle_option(const enum option opt)
251 {
252   ...
253 }
254
255 Note: The compiler will _not_ force you to pass an entry from the enum
256 as an argument to this function; this recommendation serves only to make
257 the code clearer and somewhat self-documenting. In addition, when using
258 switch/case blocks that switch on enum values, the compiler will warn
259 you if you forget to handle one or more of the enum values, which can be
260 handy.
261
262 * String handling
263 -----------------
264
265 Don't use strncpy for copying whole strings; it does not guarantee that the
266 output buffer will be null-terminated. Use ast_copy_string instead, which
267 is also slightly more efficient (and allows passing the actual buffer
268 size, which makes the code clearer).
269
270 Don't use ast_copy_string (or any length-limited copy function) for copying
271 fixed (known at compile time) strings into buffers, if the buffer is something
272 that has been allocated in the function doing the copying. In that case, you
273 know at the time you are writing the code whether the buffer is large enough
274 for the fixed string or not, and if it's not, your code won't work anyway!
275 Use strcpy() for this operation, or directly set the first two characters
276 of the buffer if you are just trying to store a one-character string in the
277 buffer. If you are trying to 'empty' the buffer, just store a single
278 NULL character ('\0') in the first byte of the buffer; nothing else is
279 needed, and any other method is wasteful.
280
281 In addition, if the previous operations in the function have already
282 determined that the buffer in use is adequately sized to hold the string
283 you wish to put into it (even if you did not allocate the buffer yourself),
284 use a direct strcpy(), as it can be inlined and optimized to simple
285 processor operations, unlike ast_copy_string().
286
287 * Use of functions
288 ------------------
289
290 When making applications, always ast_strdupa(data) to a local pointer if
291 you intend to parse the incoming data string.
292
293         if (data)
294                 mydata = ast_strdupa(data);
295
296
297 - Separating arguments to dialplan applications and functions
298 Use ast_app_separate_args() to separate the arguments to your application
299 once you have made a local copy of the string.
300
301 - Parsing strings with strsep
302 Use strsep() for parsing strings when possible; there is no worry about
303 're-entrancy' as with strtok(), and even though it modifies the original
304 string (which the man page warns about), in many cases that is exactly
305 what you want!
306
307 - Create generic code!
308 If you do the same or a similar operation more than one time, make it a
309 function or macro.
310
311 Make sure you are not duplicating any functionality already found in an
312 API call somewhere.  If you are duplicating functionality found in 
313 another static function, consider the value of creating a new API call 
314 which can be shared.
315
316 * Handling of pointers and allocations
317 --------------------------------------
318
319 - Dereference or localize pointers
320 Always dereference or localize pointers to things that are not yours like
321 channel members in a channel that is not associated with the current 
322 thread and for which you do not have a lock.
323         channame = ast_strdupa(otherchan->name);
324
325 - Use const on pointer arguments if possible
326 Use const on pointer arguments which your function will not be modifying, as this 
327 allows the compiler to make certain optimizations. In general, use 'const'
328 on any argument that you have no direct intention of modifying, as it can
329 catch logic/typing errors in your code when you use the argument variable
330 in a way that you did not intend.
331
332 - Do not create your own linked list code - reuse!
333 As a common example of this point, make an effort to use the lockable
334 linked-list macros found in include/asterisk/linkedlists.h. They are
335 efficient, easy to use and provide every operation that should be
336 necessary for managing a singly-linked list (if something is missing,
337 let us know!). Just because you see other open-coded list implementations
338 in the source tree is no reason to continue making new copies of
339 that code... There are also a number of common string manipulation
340 and timeval manipulation functions in asterisk/strings.h and asterisk/time.h;
341 use them when possible.
342
343 - Avoid needless allocations!
344 Avoid needless malloc(), strdup() calls. If you only need the value in
345 the scope of your function try ast_strdupa() or declare structs on the
346 stack and pass a pointer to them. However, be careful to _never_ call
347 alloca(), ast_strdupa() or similar functions in the argument list
348 of a function you are calling; this can cause very strange stack
349 arrangements and produce unexpected behavior.
350
351 -Allocations for structures
352 When allocating/zeroing memory for a structure, use code like this:
353
354 struct foo *tmp;
355
356 ...
357
358 tmp = ast_calloc(1, sizeof(*tmp));
359
360 Avoid the combination of ast_malloc() and memset().  Instead, always use
361 ast_calloc(). This will allocate and zero the memory in a single operation. 
362 In the case that uninitialized memory is acceptable, there should be a comment
363 in the code that states why this is the case.
364
365 Using sizeof(*tmp) instead of sizeof(struct foo) eliminates duplication of the 
366 'struct foo' identifier, which makes the code easier to read and also ensures 
367 that if it is copy-and-pasted it won't require as much editing.
368
369 The ast_* family of functions for memory allocation are functionally the same.
370 They just add an Asterisk log error message in the case that the allocation
371 fails for some reason. This eliminates the need to generate custom messages
372 throughout the code to log that this has occurred.
373
374 -String Duplications
375
376 The functions strdup and strndup can *not* accept a NULL argument. This results
377 in having code like this:
378
379         if (str)
380                 newstr = strdup(str);
381         else
382                 newstr = NULL;
383
384 However, the ast_strdup and ast_strdup functions will happily accept a NULL
385 argument without generating an error.  The same code can be written as:
386         
387         newstr = ast_strdup(str);
388
389 Furthermore, it is unnecessary to have code that malloc/calloc's for the length
390 of a string (+1 for the terminating '\0') and then using strncpy to copy the
391 copy the string into the resulting buffer.  This is the exact same thing as
392 using ast_strdup.
393
394 * CLI Commands
395 --------------
396
397 New CLI commands should be named using the module's name, followed by a verb
398 and then any parameters that the command needs. For example:
399
400 *CLI> iax2 show peer <peername>
401
402 not
403
404 *CLI> show iax2 peer <peername>
405
406 * New dialplan applications/functions
407 -------------------------------------
408
409 There are two methods of adding functionality to the Asterisk
410 dialplan: applications and functions. Applications (found generally in
411 the apps/ directory) should be collections of code that interact with
412 a channel and/or user in some significant way. Functions (which can be
413 provided by any type of module) are used when the provided
414 functionality is simple... getting/retrieving a value, for
415 example. Functions should also be used when the operation is in no way
416 related to a channel (a computation or string operation, for example).
417
418 Applications are registered and invoked using the
419 ast_register_application function; see the apps/app_skel.c file for an
420 example.
421
422 Functions are registered using 'struct ast_custom_function'
423 structures and the ast_custom_function_register function.
424
425 * Doxygen API Documentation Guidelines
426 --------------------------------------
427
428 When writing Asterisk API documentation the following format should be
429 followed. Do not use the javadoc style.
430
431 /*!
432  * \brief Do interesting stuff.
433  * \param thing1 interesting parameter 1.
434  * \param thing2 interesting parameter 2.
435  *
436  * This function does some interesting stuff.
437  *
438  * \return zero on success, -1 on error.
439  */
440 int ast_interesting_stuff(int thing1, int thing2)
441 {
442         return 0;
443 }
444
445 Notice the use of the \param, \brief, and \return constructs.  These should be
446 used to describe the corresponding pieces of the function being documented.
447 Also notice the blank line after the last \param directive.  All doxygen
448 comments must be in one /*! */ block.  If the function or struct does not need
449 an extended description it can be left out.
450
451 Please make sure to review the doxygen manual and make liberal use of the \a,
452 \code, \c, \b, \note, \li and \e modifiers as appropriate.
453
454 When documenting a 'static' function or an internal structure in a module,
455 use the \internal modifier to ensure that the resulting documentation
456 explicitly says 'for internal use only'.
457
458 Structures should be documented as follows.
459
460 /*!
461  * \brief A very interesting structure.
462  */
463 struct interesting_struct
464 {
465         /*! \brief A data member. */
466         int member1;
467
468         int member2; /*!< \brief Another data member. */
469 }
470
471 Note that /*! */ blocks document the construct immediately following them
472 unless they are written, /*!< */, in which case they document the construct
473 preceding them.
474
475 * Finishing up before you submit your code
476 ------------------------------------------
477
478 - Look at the code once more
479 When you achieve your desired functionalty, make another few refactor
480 passes over the code to optimize it.
481
482 - Read the patch
483 Before submitting a patch, *read* the actual patch file to be sure that 
484 all the changes you expect to be there are, and that there are no 
485 surprising changes you did not expect. During your development, that
486 part of Asterisk may have changed, so make sure you compare with the
487 latest CVS.
488
489 - Listen to advice
490 If you are asked to make changes to your patch, there is a good chance
491 the changes will introduce bugs, check it even more at this stage.
492 Also remember that the bug marshal or co-developer that adds comments 
493 is only human, they may be in error :-)
494
495 - Optimize, optimize, optimize
496 If you are going to reuse a computed value, save it in a variable
497 instead of recomputing it over and over.  This can prevent you from 
498 making a mistake in subsequent computations, making it easier to correct
499 if the formula has an error and may or may not help optimization but 
500 will at least help readability.
501
502 Just an example (so don't over analyze it, that'd be a shame):
503
504 const char *prefix = "pre";     
505 const char *postfix = "post";
506 char *newname;
507 char *name = "data";
508
509 if (name && (newname = alloca(strlen(name) + strlen(prefix) + strlen(postfix) + 3)))
510         snprintf(newname, strlen(name) + strlen(prefix) + strlen(postfix) + 3, "%s/%s/%s", prefix, name, postfix);
511
512 ...vs this alternative:
513
514 const char *prefix = "pre";
515 const char *postfix = "post";
516 char *newname;
517 char *name = "data";
518 int len = 0;
519
520 if (name && (len = strlen(name) + strlen(prefix) + strlen(postfix) + 3) && (newname = alloca(len)))
521         snprintf(newname, len, "%s/%s/%s", prefix, name, postfix);
522
523
524 -----------------------------------------------
525 Welcome to the Asterisk development community!
526 Meet you on the asterisk-dev mailing list. 
527 Subscribe at http://lists.digium.com!
528
529 Mark Spencer, Kevin P. Fleming and 
530 the Asterisk.org Development Team