Blocked revisions 74262 via svnmerge
[asterisk/asterisk.git] / doc / CODING-GUIDELINES
old mode 100755 (executable)
new mode 100644 (file)
index 7d621b4..990f5bc
-Asterisk Patch/Coding Guidelines
+== Asterisk Patch/Coding Guidelines ==
+--------------------------------------
 
+We are looking forward to your contributions to Asterisk - the 
+Open Source PBX! As Asterisk is a large and in some parts very
+time-sensitive application, the code base needs to conform to
+a common set of coding rules so that many developers can enhance
+and maintain the code. Code also needs to be reviewed and tested
+so that it works and follows the general architecture and guide-
+lines, and is well documented.
+
+Asterisk is published under a dual-licensing scheme by Digium.
 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 a checkout
+from subversion.
+
+/usr/src/asterisk$ svn diff > mypatch
+
+If you would like to only include changes to certain files in the patch, you
+can list them in the "svn diff" command:
+
+/usr/src/asterisk$ svn diff somefile.c someotherfile.c > mypatch
+
+* General rules
+---------------
+
+- All code, filenames, function names and comments must be in ENGLISH.
+
+- Don't annotate your changes with comments like "/* JMG 4/20/04 */";
+  Comments should explain what the code does, not when something was changed
+  or who changed it. If you have done a larger contribution, make sure
+  that you are added to the CREDITS file.
+
+- Don't make unnecessary whitespace changes throughout the code.
+  If you make changes, submit them to the tracker as separate patches
+  that only include whitespace and formatting changes.
+
+- Don't use C++ type (//) comments.
+
+- Try to match the existing formatting of the file you are working on.
+
+- Use spaces instead of tabs when aligning in-line comments or #defines (this makes 
+  your comments aligned even if the code is viewed with another tabsize)
+
+* Declaration of functions and variables
+----------------------------------------
+
+- Do not declare variables mid-block (e.g. like recent GNU compilers support)
+  since it is harder to read and not portable to GCC 2.95 and others.
+
+- 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 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.
+
+- Strings that are coming from input should not be used as a first argument to
+  a formatted *printf function.
 
-Don't annotate your changes with comments like "/* JMG 4/20/04 */";
-Comments should explain what the code does, not when something was changed
-or who changed it.
+* Use the internal API
+----------------------
 
-Don't make unnecessary whitespace changes throughout the code.
+- Make sure you are aware of the string and data handling functions that exist
+  within Asterisk to enhance portability and in some cases to produce more
+  secure and thread-safe code. Check utils.c/utils.h for these.
 
-Don't use C++ type (//) comments.
+- If you need to create a detached thread, use the ast_pthread_create_detached()
+  normally or ast_pthread_create_detached_background() for a thread with a smaller
+  stack size.  This reduces the replication of the code to handle the pthread_attr_t
+  structure.
 
-Try to match the existing formatting of the file you are working on.
+* Code formatting
+-----------------
 
-Functions and variables that are not intended to be global must be
-declared static.
+Roughly, Asterisk code formatting guidelines are generally equivalent to the 
+following:
+
+# indent -i4 -ts4 -br -brs -cdw -lp -ce -nbfda -npcs -nprs -npsl -nbbo -saf -sai -saw -cs -l90 foo.c
+
+this means in verbose:
+ -i4:    indent level 4
+ -ts4:   tab size 4
+ -br:    braces on if line
+ -brs:   braces on struct decl line
+ -cdw:   cuddle do while
+ -lp:    line up continuation below parenthesis
+ -ce:    cuddle else
+ -nbfda: dont break function decl args
+ -npcs:  no space after function call names
+ -nprs:  no space after parentheses
+ -npsl:  dont break procedure type
+ -saf:   space after for
+ -sai:   space after if
+ -saw:   space after while
+ -cs:    space after cast
+ -ln90:  line length 90 columns
 
 Function calls and arguments should be spaced in a consistent way across
 the codebase.
-GOOD: foo(arg1, arg2);
-GOOD: foo(arg1,arg2);  /* Acceptable but not preferred */
-BAD: foo (arg1, arg2);
-BAD: foo( arg1, arg2 );
-BAD: foo(arg1, arg2,arg3);
+       GOOD: foo(arg1, arg2);
+       GOOD: foo(arg1,arg2);   /* Acceptable but not preferred */
+       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:
+- Functions:
 int foo(int a, char *s)
 {
        return 0;
 }
 
-If statements:
+- If statements:
 if (foo) {
        bar();
 } else {
        blah();
 }
 
-Case statements:
+- Case statements:
 switch (foo) {
-       case BAR:
-               blah();
-               break;
-       case OTHER:
-               other();
-               break;
+case BAR:
+       blah();
+       break;
+case OTHER:
+       other();
+       break;
 }
+
+- No nested statements without braces, e.g.:
+
+for (x = 0; x < 5; x++)
+       if (foo) 
+               if (bar)
+                       baz();
+
+instead do:
+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.
+
+- Labels/goto are acceptable
+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.
+       
+- Never use an uninitialized variable
+Make sure you never use an uninitialized variable.  The compiler will 
+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 cast 'void *'
+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.
+
+* Function naming
+-----------------
+
+All public functions (those not marked 'static'), must be named "ast_<something>"
+and have a descriptive name.
+
+As an example, suppose you wanted to take a local function "find_feature", defined
+as static in a file, and used only in that file, and make it public, and use it
+in other files. You will have to remove the "static" declaration and define a 
+prototype in an appropriate header file (usually in include/asterisk). A more
+specific name should be given, such as "ast_find_call_feature".
+
+* Variable naming
+-----------------
+
+- Global variables
+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, 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 un-necessary typedef's
+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 instead of #define where possible
+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. In addition, when using
+switch/case blocks that switch on enum values, the compiler will warn
+you if you forget to handle one or more of the enum values, which can be
+handy.
+
+* String handling
+-----------------
+
+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 to 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().
+
+* Use of functions
+------------------
+
+When making applications, always ast_strdupa(data) to a local pointer if
+you intend to parse the incoming data string.
+
+       if (data)
+               mydata = ast_strdupa(data);
+
+
+- Separating arguments to dialplan applications and functions
+Use ast_app_separate_args() to separate the arguments to your application
+once you have made a local copy of the string.
+
+- Parsing strings with strsep
+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!
+
+- Create generic code!
+If you do the same or a similar operation more than one time, make it a
+function or macro.
+
+Make sure you are not duplicating any functionality already found in an
+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.
+
+* Handling of pointers and allocations
+--------------------------------------
+
+- Dereference or localize pointers
+Always dereference 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);
+
+- Use const on pointer arguments if possible
+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.
+
+- Do not create your own linked list code - reuse!
+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.
+
+- Avoid needless allocations!
+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.
+
+-Allocations for structures
+When allocating/zeroing memory for a structure, use code like this:
+
+struct foo *tmp;
+
+...
+
+tmp = ast_calloc(1, sizeof(*tmp));
+
+Avoid the combination of ast_malloc() and memset().  Instead, always use
+ast_calloc(). This will allocate and zero the memory in a single operation. 
+In the case that uninitialized memory is acceptable, there should be a comment
+in the code that states why this is the case.
+
+Using sizeof(*tmp) instead of sizeof(struct foo) 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.
+
+The ast_* family of functions for memory allocation are functionally the same.
+They just add an Asterisk log error message in the case that the allocation
+fails for some reason. This eliminates the need to generate custom messages
+throughout the code to log that this has occurred.
+
+-String Duplications
+
+The functions strdup and strndup can *not* accept a NULL argument. This results
+in having code like this:
+
+       if (str)
+               newstr = strdup(str);
+       else
+               newstr = NULL;
+
+However, the ast_strdup and ast_strdup functions will happily accept a NULL
+argument without generating an error.  The same code can be written as:
+       
+       newstr = ast_strdup(str);
+
+Furthermore, it is unnecessary to have code that malloc/calloc's for the length
+of a string (+1 for the terminating '\0') and then using strncpy to copy the
+copy the string into the resulting buffer.  This is the exact same thing as
+using ast_strdup.
+
+* CLI Commands
+--------------
+
+New CLI commands should be named using the module's name, followed by a verb
+and then any parameters that the command needs. For example:
+
+*CLI> iax2 show peer <peername>
+
+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. Do not use the javadoc style.
+
+/*!
+ * \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.
+
+It is very much preferred that documentation is not done inline, as done in
+the previous example for member2.  The first reason for this is that it tends
+to encourage extremely brief, and often pointless, documentation since people
+try to keep the comment from making the line extremely long.  However, if you
+insist on using inline comments, please indent the documentation with spaces!
+That way, all of the comments are properly aligned, regardless of what tab
+size is being used for viewing the code.
+
+* Finishing up before you submit your code
+------------------------------------------
+
+- Look at the code once more
+When you achieve your desired functionality, make another few refactor
+passes over the code to optimize it.
+
+- Read the patch
+Before submitting a patch, *read* the actual patch file to be sure that 
+all the changes you expect to be there are, and that there are no 
+surprising changes you did not expect. During your development, that
+part of Asterisk may have changed, so make sure you compare with the
+latest CVS.
+
+- Listen to advice
+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.
+Also remember that the bug marshal or co-developer that adds comments 
+is only human, they may be in error :-)
+
+- Optimize, optimize, optimize
+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, making it easier to correct
+if the formula has an error and may or may not help optimization but 
+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;
+char *name = "data";
+
+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 this alternative:
+
+const char *prefix = "pre";
+const char *postfix = "post";
+char *newname;
+char *name = "data";
+int len = 0;
+
+if (name && (len = strlen(name) + strlen(prefix) + strlen(postfix) + 3) && (newname = alloca(len)))
+       snprintf(newname, len, "%s/%s/%s", prefix, name, postfix);
+
+* Creating new manager events?
+------------------------------
+If you create new AMI events, please read manager.txt. Do not re-use
+existing headers for new purposes, but please re-use existing headers
+for the same type of data.
+
+Manager events that signal a status are required to have one
+event name, with a status header that shows the status.
+The old style, with one event named "ThisEventOn" and another named
+"ThisEventOff", is no longer approved.
+
+Check manager.txt for more information on manager and existing
+headers. Please update this file if you add new headers.
+
+-----------------------------------------------
+Welcome to the Asterisk development community!
+Meet you on the asterisk-dev mailing list. 
+Subscribe at http://lists.digium.com!
+
+Mark Spencer, Kevin P. Fleming and 
+the Asterisk.org Development Team