X11workbench Toolkit Coding Style Guidelines

CODE APPEARANCE

Coding Style for this project uses Allman Style indenting, NO hard tab characters, and with 2 (two) spaces per indent. See http://en.wikipedia.org/wiki/Indent_style#Allman_style

On FreeBSD, you can rapidly convert 'less readable' code to the 'improved' (and approved) style using the following command:

indent -bl -ncdb -nce -ci3 -cli1 -c1 -d0 -di1 -dj -nfc1 -nfcb -i2 -l255 -lp -npsl -nsc -nut filename.c

With the exception of macros, ALL conditional and loop commands (if, for, while) must be on a line SEPARATE from the opening curly brace, and ALL of them should have curly braces around the conditional or loop body (particularly for a single line of executed code). The only exception would be a 'for' or 'while' loop that has no body, which can have '{ }' following the 'for' or 'while' to visually indicate that there is no loop body.
The purpose for this rule is twofold: First, it is not (easily) possible to set a breakpoint on a conditional statement that is on the same line as an 'if', or within a loop if it is on the same line as a 'for' or 'while'. Second, the code is a LOT more readable when extra line feeds are thrown in, even though it means you have an 'otherwise blank' line. If you are THAT concerned about vertical white space, use it by putting a comment after the '{' or '}'.

White space between 'if' 'for' 'while' and the leading paranthesis '(' is discouraged. The same is true for function calls and function definitions. It does not really add to the readability. Multiple lines properly indented, however, CAN make things more readable.
'else' is NEVER on the same line as '{' or '}'. NEVER. Not negotiable. 'else if' is OK.
But NEVER should '} else {' EVER exist ANYWHERE in this code base. It's just plain UGLY.
And for what it's worth, 'if (conditional) {' is nearly as bad. Put the '{' on its OWN line, as I pointed out above.

When the line length of a single code statement exceeds the typical width of an editor (80 to 100 columns), you should split it onto multiple lines. Indentation of the lines should follow a reasonable method that helps you to 'visually group' things. Here are a few examples:

for(i1=0,p1=buffer,pEnd=buffer + sizeof(buffer);
    p1 < pEnd;
    p1++, i1++)
{
  ...
}

if(p1 &&
   ((*p1 == '\'' && (!p1[1] || p1[1] == '\'')) ||
    (*p1 == '"' && (!p1[1] || p1[1] == '"'))))
{
  ...
}

SECURITY AND PERFORMANCE

Avoid ANY possibility of buffer overruns. Use 'fgets' and not 'gets', do parameter checking and parse 'the hard way' instead of using scanf or sscanf for string parameters, etc.
If 'proof of concept' or 'under development' code temporarily uses 'the wrong method', mark the code with a 'TODO' comment to indicate that it needs to be re-factored for reliability.

Avoid excessive use of allocated memory. If you need a lot of small memory blocks, create a structure instead, and allocate THAT way. If the structure has an array in it, the array should be the final member of the structure, declared with a single element, and the actual size of the structure should be large enough to accomodate the entire array. There are already a lot of examples of dynamically sized array structures within the code that behave this way.
In cases where an object may be accessed by a worker thread after the main thread has destroyed it, you should use reference counting to prevent accessing 'free'd memory blocks.

There is a set of generic platform-independent sub-allocation utilities, WBAlloc(), WBReAlloc() and WBFree(). In those cases where you really DO need to allocate a lot of dynamic memory blocks, use these functions in lieu of 'malloc()' 'realloc()' or 'free()'. Contributed code should not abuse these functions, but INSTEAD make use of them ONLY when actually needed, and get cleaned up properly. There should NEVER be any 'use after free' or 'memory leak' problems in the toolkit or in the X11 Workbench.

Preventing 'use after free' can be a simple discipline of assigning any pointer to NULL right after calling WBFree(), even if there is no code that might use the pointer (for now, at least). Code that might be added later by others may attempt to use this pointer, assuming it is valid. Assigning it to NULL is therefore pre-emptive in this case, forcing a crash to occur instead of pointer re-use, which is FAR more preferable, as it would likely be caught early on.

All functions must be declared in the form of 'a prototype', as per ANSI standards. Functions without any arguments must have '(void)' rather than '()'. All function return types are to be declared (no assumptions), and inline functions MUST be declared 'static', especially in header files.
Declaring no-parameter functions with a prototype or using '(void)' for the parameter list, will work around those casees where a function is declared earlier in the code, without an actual prototype, and you get a warning that it is "not a prototype".

You should do a test-build of the code on multiple platforms, both debug and release, and eliminate all compile warnings. In some cases a warning will be generated on a 32-bit system but not on a 64-bit system (or vice versa). In other cases a warning may be generated by a compiler on one operating system but not another. Building on multiple platforms helps to address this.

OBJECTS AND SHARED DATA

C language code can be 'object oriented' too, if designed properly, by using object-oriented principles in the design and use of structures as 'objects'. Several structures are already being abstracted to 'hide' members that are for internal use only, and others have function 'vtable' structures associated with them. If you create a persistent structure that is to be shared or abstracted, you should have a 'Constructor' API function and a corresponding 'Destroy' API function (as needed).
For shared objects, particularly those that may be used simultaneously by different threads, you should have thread-safe entry/exit code (as needed) and some form of reference counting, such as 'AddRef/Release' functions, in which 'the final Release' will actually destroy the object. This is similar to the object reference count handling done by both CORBA and OLE 2.0
CODE COMMENTS AND NAMING CONVENTIONS

Code comments are always welcome. Unless the comments are trivial, there is no such thing as "too many comments", and even trivial ones might be welcome at times. Single-line comments are preferred except for '#define', unless you need a commented area to 'stand out' more.
API functions and structures must have minimal doxygen comments describing the functions, parameters, and structure members. They should also be members of functional groups. No exceptions. You should also test the resulting documentation to make sure it's properly formatted and that all of the links work correctly.

Code should be as self-documenting as possible. Non-trivial variables should be declared using descriptive names. Hungarian notation can be helpful, but is not required. Single-character variable names are DIFFICULT TO CHANGE without manual effort and should be AVOIDED, ALWAYS!
Use of non-descriptive variables such as 'i1' or 'p3' is ok so long as its usage is intuitively obvious within the code. If any confusion might arise from a casual observer, use more descriptive variable names. Common sense should be the rule here.

'CamelHump' names should capitalize in a manner that helps understand where the word divisions are. Similarly, underscores can be used for 'all_lower_case' names for the same purpose. Generally, API functions are 'CamelHump', utilities and trivial functions are 'all_lower_case', '#define's are ALL_CAPS, and enumerations are prefixed with the enumeration name. API functions should also begin with something that identifies the subsystem, such as 'WB' or 'FW' or 'Dlg'. This is NOT a 'hard' rule, but it DOES help minimize confusion and name collisions.
It is also typical to prefix '_' or '__' on alias names or internal-use functions. Such names and functions should not be publically used except as part of a typedef. Example, a structure name of 'my_struct' typedef'd as MY_STRUCT. Members of 'MY_STRUCT' (such as linked list pointers) are likely to refer to the structure name itself, and not the typedef name, so both names are needed.

'Magic numbers' are a necessary evil sometimes. Either document where the number came from, or use a '#define' with a descriptive name, or use BOTH. A '#define' placed in the code within a function is acceptable as long as it's only used within that function.
GOOD AND BAD CODE EXAMPLES:

// ----------
// -- GOOD --
// ----------

#define MY_BUFFER_SIZE 256
int WBMain(int argc, char *argv[])
{
  int i1;
  char buffer[MY_BUFFER_SIZE];

  // use of 'fgets' with specific buffer size
  // no danger of buffer overflow this way

  if(fgets(buffer, sizeof(buffer), stdin))
  {
    i1 = strlen(buffer);

    fprintf(stdout, "Hello World %d\n", i1);
    // using fprintf rather than printf, but specifying 'stdout'
    // If need be, it can be easily re-directed to a different file
    // it also clarifies that output will be to 'stdout'.

    return 0; // standard 'success' return value of zero
  }
  else // proper error/exception handling
  {
    fprintf(stderr, "fgets returned NULL\n");
    // error messages to 'stderr'

    return 1; // return an error code
  }
}

// ---------
// -- BAD --
// ---------

main() {
  // ugly '{' on same line as function - this is supposed to be ANSI C, and NOT K&R nor PASCAL!
  // function parameters missing, should be (void) or properly declared, even if unused
  // missing return type on 'main' (int assumed, don't rely on it!)

  int a;           // single-character name, no indication of type from the name
  char b[256];     // single-character name, 'magic number' buffer size

  if (gets(b)) a = strlen(b);    // 'if' on same line as conditional code (can't set debug breakpoint!)
                                 // 'gets' has no buffer size checking, buffer overrun possible
                                 // no '{' '}' around the conditional code block (bad style)

  printf ("Hello, World %d\n", a); // use of 'printf' discouraged; 'fprintf' with 'stdout' or 'stderr'
                                   // is preferred.  Use stderr for messages unless it's "the actual output"

  // sloppy error handling ('a' may be un-initialized)

  // no return value specified
}