a small upgrade to the coding standard, and an update to the code that triggered...
[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 a checkout
18 from subversion.
19
20 /usr/src/asterisk$ svn diff > mypatch
21
22 If you would like to only include changes to certain files in the patch, you
23 can list them in the "svn diff" command:
24
25 /usr/src/asterisk$ svn diff somefile.c someotherfile.c > mypatch
26
27 * General rules
28 ---------------
29
30 - All code, filenames, function names and comments must be in ENGLISH.
31
32 - Don't annotate your changes with comments like "/* JMG 4/20/04 */";
33   Comments should explain what the code does, not when something was changed
34   or who changed it. If you have done a larger contribution, make sure
35   that you are added to the CREDITS file.
36
37 - Don't make unnecessary whitespace changes throughout the code.
38   If you make changes, submit them to the tracker as separate patches
39   that only include whitespace and formatting changes.
40
41 - Don't use C++ type (//) comments.
42
43 - Try to match the existing formatting of the file you are working on.
44
45 - Use spaces instead of tabs when aligning in-line comments or #defines (this makes 
46   your comments aligned even if the code is viewed with another tabsize)
47
48 * Declaration of functions and variables
49 ----------------------------------------
50
51 - Do not declare variables mid-block (e.g. like recent GNU compilers support)
52   since it is harder to read and not portable to GCC 2.95 and others.
53
54 - Functions and variables that are not intended to be used outside the module
55   must be declared static.
56
57 - When reading integer numeric input with scanf (or variants), do _NOT_ use '%i'
58   unless you specifically want to allow non-base-10 input; '%d' is always a better
59   choice, since it will not silently turn numbers with leading zeros into base-8.
60
61 - Strings that are coming from input should not be used as a first argument to
62   a formatted *printf function.
63
64 * Use the internal API
65 ----------------------
66
67 - Make sure you are aware of the string and data handling functions that exist
68   within Asterisk to enhance portability and in some cases to produce more
69   secure and thread-safe code. Check utils.c/utils.h for these.
70
71
72 * Code formatting
73 -----------------
74
75 Roughly, Asterisk code formatting guidelines are generally equivalent to the 
76 following:
77
78 # indent -i4 -ts4 -br -brs -cdw -lp -ce -nbfda -npcs -nprs -npsl -nbbo -saf -sai -saw -cs -l90 foo.c
79
80 this means in verbose:
81  -i4:    indent level 4
82  -ts4:   tab size 4
83  -br:    braces on if line
84  -brs:   braces on struct decl line
85  -cdw:   cuddle do while
86  -lp:    line up continuation below parenthesis
87  -ce:    cuddle else
88  -nbfda: dont break function decl args
89  -npcs:  no space after function call names
90  -nprs:  no space after parentheses
91  -npsl:  dont break procedure type
92  -saf:   space after for
93  -sai:   space after if
94  -saw:   space after while
95  -cs:    space after cast
96  -ln90:  line length 90 columns
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 * Function naming
210 -----------------
211
212 All public functions (those not marked 'static'), must be named "ast_<something>"
213 and have a descriptive name.
214
215 As an example, suppose you wanted to take a local function "find_feature", defined
216 as static in a file, and used only in that file, and make it public, and use it
217 in other files. You will have to remove the "static" declaration and define a 
218 prototype in an appropriate header file (usually in include/asterisk). A more
219 specific name should be given, such as "ast_find_call_feature".
220
221 * Variable naming
222 -----------------
223
224 - Global variables
225 Name global variables (or local variables when you have a lot of them or
226 are in a long function) something that will make sense to aliens who
227 find your code in 100 years.  All variable names should be in lower 
228 case, except when following external APIs or specifications that normally
229 use upper- or mixed-case variable names; in that situation, it is
230 preferable to follow the external API/specification for ease of
231 understanding.
232
233 Make some indication in the name of global variables which represent
234 options that they are in fact intended to be global.
235  e.g.: static char global_something[80]
236
237 - Don't use un-necessary typedef's
238 Don't use 'typedef' just to shorten the amount of typing; there is no substantial
239 benefit in this:
240 struct foo { int bar; }; typedef foo_t struct foo;
241
242 In fact, don't use 'variable type' suffixes at all; it's much preferable to
243 just type 'struct foo' rather than 'foo_s'.
244
245 - Use enums instead of #define where possible
246 Use enums rather than long lists of #define-d numeric constants when possible;
247 this allows structure members, local variables and function arguments to
248 be declared as using the enum's type. For example:
249
250 enum option {
251   OPT_FOO = 1
252   OPT_BAR = 2
253   OPT_BAZ = 4
254 };
255
256 static enum option global_option;
257
258 static handle_option(const enum option opt)
259 {
260   ...
261 }
262
263 Note: The compiler will _not_ force you to pass an entry from the enum
264 as an argument to this function; this recommendation serves only to make
265 the code clearer and somewhat self-documenting. In addition, when using
266 switch/case blocks that switch on enum values, the compiler will warn
267 you if you forget to handle one or more of the enum values, which can be
268 handy.
269
270 * String handling
271 -----------------
272
273 Don't use strncpy for copying whole strings; it does not guarantee that the
274 output buffer will be null-terminated. Use ast_copy_string instead, which
275 is also slightly more efficient (and allows passing the actual buffer
276 size, which makes the code clearer).
277
278 Don't use ast_copy_string (or any length-limited copy function) for copying
279 fixed (known at compile time) strings into buffers, if the buffer is something
280 that has been allocated in the function doing the copying. In that case, you
281 know at the time you are writing the code whether the buffer is large enough
282 for the fixed string or not, and if it's not, your code won't work anyway!
283 Use strcpy() for this operation, or directly set the first two characters
284 of the buffer if you are just trying to store a one-character string in the
285 buffer. If you are trying to 'empty' the buffer, just store a single
286 NULL character ('\0') in the first byte of the buffer; nothing else is
287 needed, and any other method is wasteful.
288
289 In addition, if the previous operations in the function have already
290 determined that the buffer in use is adequately sized to hold the string
291 you wish to put into it (even if you did not allocate the buffer yourself),
292 use a direct strcpy(), as it can be inlined and optimized to simple
293 processor operations, unlike ast_copy_string().
294
295 * Use of functions
296 ------------------
297
298 When making applications, always ast_strdupa(data) to a local pointer if
299 you intend to parse the incoming data string.
300
301         if (data)
302                 mydata = ast_strdupa(data);
303
304
305 - Separating arguments to dialplan applications and functions
306 Use ast_app_separate_args() to separate the arguments to your application
307 once you have made a local copy of the string.
308
309 - Parsing strings with strsep
310 Use strsep() for parsing strings when possible; there is no worry about
311 're-entrancy' as with strtok(), and even though it modifies the original
312 string (which the man page warns about), in many cases that is exactly
313 what you want!
314
315 - Create generic code!
316 If you do the same or a similar operation more than one time, make it a
317 function or macro.
318
319 Make sure you are not duplicating any functionality already found in an
320 API call somewhere.  If you are duplicating functionality found in 
321 another static function, consider the value of creating a new API call 
322 which can be shared.
323
324 * Handling of pointers and allocations
325 --------------------------------------
326
327 - Dereference or localize pointers
328 Always dereference or localize pointers to things that are not yours like
329 channel members in a channel that is not associated with the current 
330 thread and for which you do not have a lock.
331         channame = ast_strdupa(otherchan->name);
332
333 - Use const on pointer arguments if possible
334 Use const on pointer arguments which your function will not be modifying, as this 
335 allows the compiler to make certain optimizations. In general, use 'const'
336 on any argument that you have no direct intention of modifying, as it can
337 catch logic/typing errors in your code when you use the argument variable
338 in a way that you did not intend.
339
340 - Do not create your own linked list code - reuse!
341 As a common example of this point, make an effort to use the lockable
342 linked-list macros found in include/asterisk/linkedlists.h. They are
343 efficient, easy to use and provide every operation that should be
344 necessary for managing a singly-linked list (if something is missing,
345 let us know!). Just because you see other open-coded list implementations
346 in the source tree is no reason to continue making new copies of
347 that code... There are also a number of common string manipulation
348 and timeval manipulation functions in asterisk/strings.h and asterisk/time.h;
349 use them when possible.
350
351 - Avoid needless allocations!
352 Avoid needless malloc(), strdup() calls. If you only need the value in
353 the scope of your function try ast_strdupa() or declare structs on the
354 stack and pass a pointer to them. However, be careful to _never_ call
355 alloca(), ast_strdupa() or similar functions in the argument list
356 of a function you are calling; this can cause very strange stack
357 arrangements and produce unexpected behavior.
358
359 -Allocations for structures
360 When allocating/zeroing memory for a structure, use code like this:
361
362 struct foo *tmp;
363
364 ...
365
366 tmp = ast_calloc(1, sizeof(*tmp));
367
368 Avoid the combination of ast_malloc() and memset().  Instead, always use
369 ast_calloc(). This will allocate and zero the memory in a single operation. 
370 In the case that uninitialized memory is acceptable, there should be a comment
371 in the code that states why this is the case.
372
373 Using sizeof(*tmp) instead of sizeof(struct foo) eliminates duplication of the 
374 'struct foo' identifier, which makes the code easier to read and also ensures 
375 that if it is copy-and-pasted it won't require as much editing.
376
377 The ast_* family of functions for memory allocation are functionally the same.
378 They just add an Asterisk log error message in the case that the allocation
379 fails for some reason. This eliminates the need to generate custom messages
380 throughout the code to log that this has occurred.
381
382 -String Duplications
383
384 The functions strdup and strndup can *not* accept a NULL argument. This results
385 in having code like this:
386
387         if (str)
388                 newstr = strdup(str);
389         else
390                 newstr = NULL;
391
392 However, the ast_strdup and ast_strdup functions will happily accept a NULL
393 argument without generating an error.  The same code can be written as:
394         
395         newstr = ast_strdup(str);
396
397 Furthermore, it is unnecessary to have code that malloc/calloc's for the length
398 of a string (+1 for the terminating '\0') and then using strncpy to copy the
399 copy the string into the resulting buffer.  This is the exact same thing as
400 using ast_strdup.
401
402 * CLI Commands
403 --------------
404
405 New CLI commands should be named using the module's name, followed by a verb
406 and then any parameters that the command needs. For example:
407
408 *CLI> iax2 show peer <peername>
409
410 not
411
412 *CLI> show iax2 peer <peername>
413
414 * New dialplan applications/functions
415 -------------------------------------
416
417 There are two methods of adding functionality to the Asterisk
418 dialplan: applications and functions. Applications (found generally in
419 the apps/ directory) should be collections of code that interact with
420 a channel and/or user in some significant way. Functions (which can be
421 provided by any type of module) are used when the provided
422 functionality is simple... getting/retrieving a value, for
423 example. Functions should also be used when the operation is in no way
424 related to a channel (a computation or string operation, for example).
425
426 Applications are registered and invoked using the
427 ast_register_application function; see the apps/app_skel.c file for an
428 example.
429
430 Functions are registered using 'struct ast_custom_function'
431 structures and the ast_custom_function_register function.
432
433 * Doxygen API Documentation Guidelines
434 --------------------------------------
435
436 When writing Asterisk API documentation the following format should be
437 followed. Do not use the javadoc style.
438
439 /*!
440  * \brief Do interesting stuff.
441  * \param thing1 interesting parameter 1.
442  * \param thing2 interesting parameter 2.
443  *
444  * This function does some interesting stuff.
445  *
446  * \return zero on success, -1 on error.
447  */
448 int ast_interesting_stuff(int thing1, int thing2)
449 {
450         return 0;
451 }
452
453 Notice the use of the \param, \brief, and \return constructs.  These should be
454 used to describe the corresponding pieces of the function being documented.
455 Also notice the blank line after the last \param directive.  All doxygen
456 comments must be in one /*! */ block.  If the function or struct does not need
457 an extended description it can be left out.
458
459 Please make sure to review the doxygen manual and make liberal use of the \a,
460 \code, \c, \b, \note, \li and \e modifiers as appropriate.
461
462 When documenting a 'static' function or an internal structure in a module,
463 use the \internal modifier to ensure that the resulting documentation
464 explicitly says 'for internal use only'.
465
466 Structures should be documented as follows.
467
468 /*!
469  * \brief A very interesting structure.
470  */
471 struct interesting_struct
472 {
473         /*! \brief A data member. */
474         int member1;
475
476         int member2; /*!< \brief Another data member. */
477 }
478
479 Note that /*! */ blocks document the construct immediately following them
480 unless they are written, /*!< */, in which case they document the construct
481 preceding them.
482
483 * Finishing up before you submit your code
484 ------------------------------------------
485
486 - Look at the code once more
487 When you achieve your desired functionality, make another few refactor
488 passes over the code to optimize it.
489
490 - Read the patch
491 Before submitting a patch, *read* the actual patch file to be sure that 
492 all the changes you expect to be there are, and that there are no 
493 surprising changes you did not expect. During your development, that
494 part of Asterisk may have changed, so make sure you compare with the
495 latest CVS.
496
497 - Listen to advice
498 If you are asked to make changes to your patch, there is a good chance
499 the changes will introduce bugs, check it even more at this stage.
500 Also remember that the bug marshal or co-developer that adds comments 
501 is only human, they may be in error :-)
502
503 - Optimize, optimize, optimize
504 If you are going to reuse a computed value, save it in a variable
505 instead of recomputing it over and over.  This can prevent you from 
506 making a mistake in subsequent computations, making it easier to correct
507 if the formula has an error and may or may not help optimization but 
508 will at least help readability.
509
510 Just an example (so don't over analyze it, that'd be a shame):
511
512 const char *prefix = "pre";     
513 const char *postfix = "post";
514 char *newname;
515 char *name = "data";
516
517 if (name && (newname = alloca(strlen(name) + strlen(prefix) + strlen(postfix) + 3)))
518         snprintf(newname, strlen(name) + strlen(prefix) + strlen(postfix) + 3, "%s/%s/%s", prefix, name, postfix);
519
520 ...vs this alternative:
521
522 const char *prefix = "pre";
523 const char *postfix = "post";
524 char *newname;
525 char *name = "data";
526 int len = 0;
527
528 if (name && (len = strlen(name) + strlen(prefix) + strlen(postfix) + 3) && (newname = alloca(len)))
529         snprintf(newname, len, "%s/%s/%s", prefix, name, postfix);
530
531 * Creating new manager events?
532 ------------------------------
533 If you create new AMI events, please read manager.txt. Do not re-use
534 existing headers for new purposes, but please re-use existing headers
535 for the same type of data.
536
537 Manager events that signal a status are required to have one
538 event name, with a status header that shows the status.
539 The old style, with one event named "ThisEventOn" and another named
540 "ThisEventOff", is no longer approved.
541
542 Check manager.txt for more information on manager and existing
543 headers. Please update this file if you add new headers.
544
545 -----------------------------------------------
546 Welcome to the Asterisk development community!
547 Meet you on the asterisk-dev mailing list. 
548 Subscribe at http://lists.digium.com!
549
550 Mark Spencer, Kevin P. Fleming and 
551 the Asterisk.org Development Team