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