Mozilla Coding Style Guide
This document attempts to explain the basic styles and patterns that are used in the Mozilla codebase. New code should try to conform to these standards so that it is as easy to maintain as existing code. Of course every rule has an exception, but it's important to know the rules nonetheless!
This is particularly directed at people new to the Mozilla codebase, who are in the process of getting their code reviewed. Before getting a review, please read over this document and make sure your code conforms to the recommendations here.
General C/C++ Practices
COM and pointers
IDL
Error Handling
Strings
Naming and Formatting code
General C/C++ Practices
- Have you checked for compiler warnings? Warnings often point to real bugs.
- Are the changes 64-bit clean?
- Do the changes meet the C++ portability guidelines>
- Don't use NULL for pointers. On some systems it's declared as void * and causes a compile warning when assigned to a pointer. Use 0 or nsnull instead.
- When testing a pointer, use !myPtr or (myPtr); don't use myPtr != nsnull or myPtr == nsnull.
- Do not compare == PR_TRUE or PR_FALSE. Use (x) or (!x) instead. == PR_TRUE, in fact, is *different* from if (x)!
- Don't put an else right after a return. Delete the else, it's unnecessary and increases indentation level.
- Always check the return value of new for null.
- Don't leave debug printf()s lying around.
- Use JavaDoc-style comments in any new class header files.
- When fixing a problem, check to see if the problem occurs elsewhere in the same file, and fix it everywhere if possible.
- On whether to use
nsFoo aFoo (bFoo)
ornsFoo aFoo = bFoo
: For first tier platforms, although the former is theoretically better, there is probably no reason to prefer it over the latter form. This is good, since everyone agrees, the form with "=" looks better. More data for second tier platforms would be good. - Forward declare classes in your header files instead of including them
whenever possible. For example, if you have an interface with a
void DoSomething(nsIContent* aContent)
function, forward declare withclass nsIContent;
instead of#include "nsIContent.h"
COM and pointers
- Use nsCOMPtr<>
If you don't know how to use it, start looking in the code for examples. The general rule is that the very act of typing NS_RELEASE should be a signal to you to question your code: "Should I be using nsCOMPtr here?". Generally the only valid use of NS_RELEASE are when you are storing refcounted pointers in a long-lived datastructure. - Declare new XPCOM interfaces using XPIDL so they will be scriptable.
- Use nsCOMPtr for strong references, and nsWeakPtr for weak references.
- String arguments to functions should be declared as nsAString.
- Use str.IsEmpty() instead of str.Length() == 0.
- Don't use QueryInterface directly. Use CallQueryInterface or do_QueryInterface instead.
- nsresult should be declared as rv. Not res, not result, not foo.
- For constant strings, use NS_LITERAL_STRING("...") instead of NS_ConvertASCIItoUCS2("..."), AssignWithConversion("..."), EqualsWithConversion("..."), or nsAutoString()
- Use contractids instead of progids or class IDs.
IDL
long updateStatusBar();
This example has too many methods:
interface nsIFoo : nsISupports { long getLength(); void setLength(in long length); long getColor(); };The code below will generate the exact same C++ signature, but is more script-friendly.
interface nsIFoo : nsISupports { attribute long length; readonly attribute long color; };
const long ERROR_UNDEFINED_VARIABLE = 1;
Error handling
- Someone may change the callee in the future to return a failure condition.
- The object in question may live on another thread, another process, or possibly even another machine. The proxy could have failed to actually make your call in the first place.
NS_ENSURE_SUCCESS(rv, rv)
and NS_ENSURE_TRUE(expr, rv)
macros in place of
if (NS_FAILED(rv)) { return rv; }
and if (!expr) { return rv; }
,
unless the failure is a normal condition (i.e.
you don't want it to assert).
rv = foo->Call1(); if (NS_SUCCEEDED(rv)) { rv = foo->Call2(); if (NS_SUCCEEDED(rv)) { rv = foo->Call3(); } } } return rv;Instead, do this:
rv = foo->Call1(); NS_ENSURE_SUCCESS(rv, rv); rv = foo->Call2(); NS_ENSURE_SUCCESS(rv, rv); rv = foo->Call3(); NS_ENSURE_SUCCESS(rv, rv);
Why? Because error handling should not obfuscate the logic of the code. The author's intent in the first example was to make 3 calls in succession, but wrapping the calls in nested if() statements obscured the most likely behavior of the code.
Consider a more complicated example that actually hides a bug:
PRBool val; rv = foo->GetBooleanValue(&val); if (NS_SUCCEEDED(rv) && val) foo->Call1(); else foo->Call2();The intent of the author may have been that foo->Call2() would only happen when val had a false value. In fact, foo->Call2() will also be called when foo->GetBooleanValue(&val) fails. This may or may not have been the author's intent, and it is not clear from this code. Here is an updated version:
PRBool val; rv = foo->GetBooleanValue(&val); if (NS_FAILED(rv)) return rv; if (val) foo->Call1(); else foo->Call2();
In this example, the author's intent is clear, and an error condition avoids both calls to foo->Call1() and foo->Call2();
Possible exceptions: Sometimes it is not fatal if a call fails. For instance, if you are notifying a series of observers that an event has fired, it might be inconsequential that one of these notifications failed:
for (i=0; i<length; i++) { // we don't care if any individual observer fails observers[i]->Observe(foo, bar, baz); }Another possibility is that you are not sure if a component exists or is installed, and you wish to continue normally if the component is not found.
nsCOMPtrservice = do_CreateInstance(NS_MYSERVICE_CID, &rv); // if the service is installed, then we'll use it if (NS_SUCCEEDED(rv)) { // non-fatal if this fails too, ignore this error service->DoSomething(); // this is important, handle this error! rv = service->DoSomethingImportant(); if (NS_FAILED(rv)) return rv; } // continue normally whether or not the service exists
Strings
nsresult foo() { nsCString bar; .. }instead:
nsresult foo() { nsCAutoString bar; .. }
static char *GetStringValue() { .. return resultString.ToNewCString(); } .. WarnUser(GetStringValue());In the above example, WarnUser will get the string allocated from resultString.ToNewCString() and throw away the pointer. The resulting value is never freed. Instead, either use the string classes to make sure your string is automatically freed when it goes out of scope, or make sure that your string is freed.
Automatic cleanup:
static void GetStringValue(nsAWritableCString& aResult) { .. aResult.Assign("resulting string"); } .. nsCAutoString warning; GetStringValue(warning); WarnUser(warning.get());
Free the string manually:
static char *GetStringValue() { .. return resultString.ToNewCString(); } .. char *warning = GetStringValue(); WarnUser(warning); nsMemory::Free(warning);
Incorrect:
nsAutoString warning; warning.AssignWithConversion("danger will robinson!"); .. foo->SetUnicodeValue(warning.get());
Correct:
NS_NAMED_LITERAL_STRING(warning,"danger will robinson!"); .. // if you'll be using the 'warning' string, you can still use it as before: foo->SetUnicodeValue(warning.get()); // alternatively, use the wide string directly: foo->SetUnicodeValue(NS_LITERAL_STRING("danger will robinson!").get());
Naming and Formatting code
Note: the following is not all set in stone, this is interim to give people a chance to look
if (...) { } else if (...) { } else { } while (...) { } do { } while (...); for (...; ...; ...) { } switch (...) { case 1: { // When you need to declare a variable in a switch, put the block in braces int var; } break; case 2: ... break; default: break; }Note the space here:
if (
. switch in particular is not quite agreed-upon ...
try really hard to find a module style for that one :)
class nsMyClass : public X, public Y { public: nsMyClass() : mVar(0) { ... }; private: int mVar; };For small functions in a class declaration, it's OK to do the above. For larger ones use something similar to method declarations below.
int nsMyClass::Method(...) { ... }
/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
Variable prefixes:
- k=constant (e.g. kNC_child)
- g=global (e.g. gPrefService)
- m=member (e.g. mLength)
- a=argument (e.g. aCount)
- s=static member (e.g. sPrefChecked)
- Macros begin with NS_, and are all caps (e.g. NS_IMPL_ISUPPORTS)
- Global (exported) functions begin with NS_ and use LeadingCaps (e.g. NS_NewISupportsArray)
Original document by Alec Flett.
Thanks to:
- pink
- smfr
- waterson
- jband
- brendan
- rogc
Additions by Akkana Peck based on discussions on IRC: thanks to: bbaetz, bz, jfrancis, jkeiser, mjudge, and sdagley for comments, and to John Keiser and JST's Reviewer Simulacrum and Brendan and Mitchell's super-review document. Last modified: Wed Oct 23 17:15 -0700 2002