more thoughts
[asterisk/asterisk.git] / doc / CODING-GUIDELINES
index b9fcfba..7b27f53 100755 (executable)
@@ -1,15 +1,21 @@
-Asterisk Patch/Coding Guidelines
+== Asterisk Patch/Coding Guidelines ==
 
 To be accepted into the codebase, all non-trivial changes must be
 disclaimed to Digium or placed in the public domain. For more information
 see http://bugs.digium.com
 
-Patches should be in the form of a unified (-u) diff.
+Patches should be in the form of a unified (-u) diff, made from the directory
+above the top-level Asterisk source directory. For example:
+
+- the base code you are working from is in ~/work/asterisk-base
+- the changes are in ~/work/asterisk-new
+
+~/work$ diff -urN asterisk-base asterisk-new
 
 All code, filenames, function names and comments must be in ENGLISH.
 
-Do not declare variables mid-function (e.g. like GNU lets you) since it is
-harder to read and not portable to GCC 2.95 and others.
+Do not declare variables mid-function (e.g. like recent  GNU compilers support)
+since it is harder to read and not portable to GCC 2.95 and others.
 
 Don't annotate your changes with comments like "/* JMG 4/20/04 */";
 Comments should explain what the code does, not when something was changed
@@ -21,14 +27,14 @@ Don't use C++ type (//) comments.
 
 Try to match the existing formatting of the file you are working on.
 
-Functions and variables that are not intended to be global must be
-declared static.
+Functions and variables that are not intended to be used outside the module
+must be declared static.
 
 When reading integer numeric input with scanf (or variants), do _NOT_ use '%i'
-unless specifically want to allow non-base-10 input; '%d' is always a better
+unless you specifically want to allow non-base-10 input; '%d' is always a better
 choice, since it will not silently turn numbers with leading zeros into base-8.
 
-Roughly, Asterisk coding guidelines are generally equivalent to the 
+Roughly, Asterisk code formatting guidelines are generally equivalent to the 
 following:
 
 # indent -i4 -ts4 -br -brs -cdw -cli0 -ce -nbfda -npcs -npsl foo.c
@@ -41,6 +47,20 @@ BAD: foo (arg1, arg2);
 BAD: foo( arg1, arg2 );
 BAD: foo(arg1, arg2,arg3);
 
+Don't treat keywords (if, while, do, return) as if they were functions;
+leave space between the keyword and the expression used (if any). For 'return',
+don't even put parentheses around the expression, since they are not
+required.
+
+There is no shortage of whitespace characters :-) Use them when they make
+the code easier to read. For example:
+
+for (str=foo;str;str=str->next)
+
+is harder to read than
+
+for (str = foo; str; str = str->next)
+
 Following are examples of how code should be formatted.
 
 Functions:
@@ -66,43 +86,129 @@ case OTHER:
        break;
 }
 
-No nested statements without braces, e.g. no:
+No nested statements without braces, e.g.:
 
-for (x=0;x<5;x++)
+for (x = 0; x < 5; x++)
        if (foo) 
                if (bar)
                        baz();
 
 instead do:
-for (x=0;x<5;x++) {
+for (x = 0; x < 5; x++) {
        if (foo) {
                if (bar)
                        baz();
        }
 }
 
+Don't build code like this:
+
+if (foo) {
+       /* .... 50 lines of code ... */
+} else {
+       result = 0;
+       return;
+}
+
+Instead, try to minimize the number of lines of code that need to be
+indented, by only indenting the shortest case of the 'if'
+statement, like so:
+
+if !(foo) {
+       result = 0;
+       return;
+}
+
+.... 50 lines of code ....
+
+When this technique is used properly, it makes functions much easier to read
+and follow, especially those with more than one or two 'setup' operations
+that must succeed for the rest of the function to be able to execute.
 
+Proper use of this technique may occasionally result in the need for a
+label/goto combination so that error/failure conditions can exit the
+function while still performing proper cleanup. This is not a bad thing!
+Use of goto in this situation is encouraged, since it removes the need
+for excess code indenting without requiring duplication of cleanup code.
+       
 Make sure you never use an uninitialized variable.  The compiler will 
-usually warn you if you do so.
+usually warn you if you do so. However, do not go too far the other way,
+and needlessly initialize variables that do not require it. If the first
+time you use a variable in a function is to store a value there, then
+initializing it at declaration is pointless, and will generate extra
+object code and data in the resulting binary with no purpose. When in doubt,
+trust the compiler to tell you when you need to initialize a variable;
+if it does not warn you, initialization is not needed.
+
+Do not explicitly cast 'void *' into any other type, nor should you cast any
+other type into 'void *'. Implicit casts to/from 'void *' are explicitly
+allowed by the C specification. This means the results of malloc(), calloc(),
+alloca(), and similar functions do not _ever_ need to be cast to a specific
+type, and when you are passing a pointer to (for example) a callback function
+that accepts a 'void *' you do not need to cast into that type.
 
 Name global variables (or local variables when you have a lot of them or
 are in a long function) something that will make sense to aliens who
 find your code in 100 years.  All variable names should be in lower 
-case.
+case, except when following external APIs or specifications that normally
+use upper- or mixed-case variable names; in that situation, it is
+preferable to follow the external API/specification for ease of
+understanding.
 
 Make some indication in the name of global variables which represent
 options that they are in fact intended to be global.
  e.g.: static char global_something[80]
 
+Don't use 'typedef' just to shorten the amount of typing; there is no substantial
+benefit in this:
+
+struct foo {
+       int bar;
+};
+typedef foo_t struct foo;
+
+In fact, don't use 'variable type' suffixes at all; it's much preferable to
+just type 'struct foo' rather than 'foo_s'.
+
+Use enums rather than long lists of #define-d numeric constants when possible;
+this allows structure members, local variables and function arguments to
+be declared as using the enum's type. For example:
+
+enum option {
+  OPT_FOO = 1
+  OPT_BAR = 2
+  OPT_BAZ = 4
+};
+
+static enum option global_option;
+
+static handle_option(const enum option opt)
+{
+  ...
+}
+
+Note: The compiler will _not_ force you to pass an entry from the enum
+as an argument to this function; this recommendation serves only to make
+the code clearer and somewhat self-documenting.
+
 When making applications, always ast_strdupa(data) to a local pointer if
-you intend to parse it.
- if(data)
-  mydata = ast_strdupa(data);
+you intend to parse the incoming data string.
+
+       if (data)
+               mydata = ast_strdupa(data);
+
+Use ast_separate_app_args() to separate the arguments to your application
+once you have made a local copy of the string.
+
+Use strsep() for parsing strings when possible; there is no worry about
+'re-entrancy' as with strtok(), and even though it modifies the original
+string (which the man page warns about), in many cases that is exactly
+what you want!
 
 Always derefrence or localize pointers to things that are not yours like
 channel members in a channel that is not associated with the current 
 thread and for which you do not have a lock.
- channame = ast_strdupa(otherchan->name);
+       channame = ast_strdupa(otherchan->name);
 
 If you do the same or a similar operation more than 1 time, make it a
 function or macro.
@@ -112,6 +218,16 @@ API call somewhere.  If you are duplicating functionality found in
 another static function, consider the value of creating a new API call 
 which can be shared.
 
+As a common example of this point, make an effort to use the lockable
+linked-list macros found in include/asterisk/linkedlists.h. They are
+efficient, easy to use and provide every operation that should be
+necessary for managing a singly-linked list (if something is missing,
+let us know!). Just because you see other open-coded list implementations
+in the source tree is no reason to continue making new copies of
+that code... There are also a number of common string manipulation
+and timeval manipulation functions in asterisk/strings.h and asterisk/time.h;
+use them when possible.
+
 When you achieve your desired functionalty, make another few refactor
 passes over the code to optimize it.
 
@@ -122,11 +238,14 @@ surprising changes you did not expect.
 If you are asked to make changes to your patch, there is a good chance
 the changes will introduce bugs, check it even more at this stage.
 
-Avoid needless malloc(),strdup() calls.  If you only need the value in
-the scope of your function try ast_strdupa() or declare struts static
-and pass them as a pointer with &.
+Avoid needless malloc(), strdup() calls. If you only need the value in
+the scope of your function try ast_strdupa() or declare structs on the
+stack and pass a pointer to them. However, be careful to _never_ call
+alloca(), ast_strdupa() or similar functions in the argument list
+of a function you are calling; this can cause very strange stack
+arrangements and produce unexpected behavior.
 
-If you are going to reuse a computable value, save it in a variable
+If you are going to reuse a computed value, save it in a variable
 instead of recomputing it over and over.  This can prevent you from 
 making a mistake in subsequent computations, make it easier to correct
 if the formula has an error and may or may not help optimization but 
@@ -134,29 +253,75 @@ will at least help readability.
 
 Just an example, so don't over analyze it, that'd be a shame:
 
-
 const char *prefix = "pre";    
 const char *postfix = "post";
-char *newname = NULL;
+char *newname;
 char *name = "data";
 
-if (name && (newname = (char *) alloca(strlen(name) + strlen(prefix) + strlen(postfix) + 3)))
+if (name && (newname = alloca(strlen(name) + strlen(prefix) + strlen(postfix) + 3)))
        snprintf(newname, strlen(name) + strlen(prefix) + strlen(postfix) + 3, "%s/%s/%s", prefix, name, postfix);
 
 vs
 
 const char *prefix = "pre";
 const char *postfix = "post";
-char *newname = NULL;
+char *newname;
 char *name = "data";
 int len = 0;
 
-if (name && (len = strlen(name) + strlen(prefix) + strlen(postfix) + 3) && (newname = (char *) alloca(len)))
+if (name && (len = strlen(name) + strlen(prefix) + strlen(postfix) + 3) && (newname = alloca(len)))
        snprintf(newname, len, "%s/%s/%s", prefix, name, postfix);
 
 
-Use const on pointers which your function will not be modifying, as this 
-allows the compiler to make certain optimizations.
+Use const on pointer arguments which your function will not be modifying, as this 
+allows the compiler to make certain optimizations. In general, use 'const'
+on any argument that you have no direct intention of modifying, as it can
+catch logic/typing errors in your code when you use the argument variable
+in a way that you did not intend.
+
+Don't use strncpy for copying whole strings; it does not guarantee that the
+output buffer will be null-terminated. Use ast_copy_string instead, which
+is also slightly more efficient (and allows passing the actual buffer
+size, which makes the code clearer).
+
+Don't use ast_copy_string (or any length-limited copy function) for copying
+fixed (known at compile time) strings into buffers, if the buffer is something
+that has been allocated in the function doing the copying. In that case, you
+know at the time you are writing the code whether the buffer is large enough
+for the fixed string or not, and if it's not, your code won't work anyway!
+Use strcpy() for this operation, or directly set the first two characters
+of the buffer if you are just trying store a one-character string in the
+buffer. If you are trying to 'empty' the buffer, just store a single
+NULL character ('\0') in the first byte of the buffer; nothing else is
+needed, and any other method is wasteful.
+
+In addition, if the previous operations in the function have already
+determined that the buffer in use is adequately sized to hold the string
+you wish to put into it (even if you did not allocate the buffer yourself),
+use a direct strcpy(), as it can be inlined and optimized to simple
+processor operations, unlike ast_copy_string().
+
+When allocating/zeroing memory for a structure, try to use code like this:
+
+struct foo *tmp;
+
+...
+
+tmp = malloc(sizeof(*tmp));
+if (tmp)
+       memset(tmp, 0, sizeof(*tmp));
+
+This eliminates duplication of the 'struct foo' identifier, which makes the
+code easier to read and also ensures that if it is copy-and-pasted it won't
+require as much editing. In fact, you can even use:
+
+struct foo *tmp;
+
+...
+
+tmp = calloc(1, sizeof(*tmp));
+
+This will allocate and zero the memory in a single operation.
 
 == CLI Commands ==
 
@@ -168,3 +333,70 @@ and then any parameters that the command needs. For example:
 not
 
 *CLI> show iax2 peer <peername>
+
+== New dialplan applications/functions ==
+
+There are two methods of adding functionality to the Asterisk
+dialplan: applications and functions. Applications (found generally in
+the apps/ directory) should be collections of code that interact with
+a channel and/or user in some significant way. Functions (which can be
+provided by any type of module) are used when the provided
+functionality is simple... getting/retrieving a value, for
+example. Functions should also be used when the operation is in no way
+related to a channel (a computation or string operation, for example).
+
+Applications are registered and invoked using the
+ast_register_application function; see the apps/app_skel.c file for an
+example.
+
+Functions are registered using 'struct ast_custom_function'
+structures and the ast_custom_function_register function.
+
+== Doxygen API Documentation Guidelines ==
+
+When writing Asterisk API documentation the following format should be
+followed.
+
+/*!
+ * \brief Do interesting stuff.
+ * \param thing1 interesting parameter 1.
+ * \param thing2 interesting parameter 2.
+ *
+ * This function does some interesting stuff.
+ *
+ * \return zero on success, -1 on error.
+ */
+int ast_interesting_stuff(int thing1, int thing2)
+{
+       return 0;
+}
+
+Notice the use of the \param, \brief, and \return constructs.  These should be
+used to describe the corresponding pieces of the function being documented.
+Also notice the blank line after the last \param directive.  All doxygen
+comments must be in one /*! */ block.  If the function or struct does not need
+an extended description it can be left out.
+
+Please make sure to review the doxygen manual and make liberal use of the \a,
+\code, \c, \b, \note, \li and \e modifiers as appropriate.
+
+When documenting a 'static' function or an internal structure in a module,
+use the \internal modifier to ensure that the resulting documentation
+explicitly says 'for internal use only'.
+
+Structures should be documented as follows.
+
+/*!
+ * \brief A very interesting structure.
+ */
+struct interesting_struct
+{
+       /*! \brief A data member. */
+       int member1;
+
+       int member2; /*!< \brief Another data member. */
+}
+
+Note that /*! */ blocks document the construct immediately following them
+unless they are written, /*!< */, in which case they document the construct
+preceding them.