For Developers‎ > ‎Coding Style‎ > ‎

C++ Dos and Don'ts

Minimize Code in Headers

Don't include unneeded headers.

If a file isn't using the symbols from some header, remove the header.  It turns out that this happens frequently in the Chromium codebase due to refactoring.

Forward declare classes instead of including headers.

You can use forward declarations in all the following cases:

class Forward;
class ObjectUser {
  public:
    ObjectUser(Forward by_value);
    void OrByPointer(Forward* by_pointer);
    void OrByReference(const Forward& by_reference);
    Forward OrReturnValue();

  private:
    Forward* pointer_member_;
    scoped_ptr<Forward> most_smart_pointer_templates_;
    std::vector<Forward> some_stl_containers_;
    std::map<std::string, Forward> values_in_maps_;
};

But you can't forward declare in other situations:

class WontCompile {
  private:
    // But you can't just:
    Forward cant_full_member_;
    std::deque<Forward> cant_some_other_stl_containers_;
};

If you can forward declare, do so.  (Exception: If it would otherwise make sense to use a type as a member by-value, don't convert it to a pointer just to be able to forward-declare the type.  See the detailed points in the Google style guide.)

Move inner classes into the implementation.

You can also forward declare classes inside a class:

class Whatever {
  public:
    /* ... */
  private:
    struct DataStruct;
    std::vector<DataStruct> data_;
};

Any headers that DataStruct needed no longer need to be included in the header file and only need to be included in the implementation. This will often let you pull includes out of the header. For reference, the syntax in the implementation file is:

struct Whatever::DataStruct {
};

Note that sometimes you can't do this because certain STL data structures require the full definition at declaration time (most notably, std::deque and the STL adapters that wrap it).

Move static implementation details to the implementation whenever possible.

If you have the class in a header file:

#include "BigImplementationDetail.h"
class PublicInterface {
  public:
    /* ... */
  private:
    static BigImplementationDetail detail_;
};

You should try to move that from a class member into the anonymous namespace in the implementation file:

namespace {
BigImplementationDetail detail_;
}  // namespace

That way, people who don't use your interface don't need to know about or care about BigImplementationDetail.

You can do this for helper functions, too.  Note that if there is more than one class in the .cc file, it can aid clarity to define your file-scope helpers in an anonymous namespace just above the class that uses them, instead of at the top of the file.


Stop inlining code in headers

BACKGROUND: Unless something is a cheap accessor or you truly need it to be inlined, don't ask for it to be inlined.  Remember that definitions inside class declarations are implicitly requested to be inlined.

class InlinedMethods {
  InlinedMethods() {
    // This constructor is equivalent to having the inline keyword in front
    // of it!
  }
  void Method() {
    // Same here!
  }
};

Stop inlining complex methods.

class DontDoThis {
  public:
    int ComputeSomething() {
      int sum =0;
      for (int i = 0; i < limit; ++i) {
        sum += OtherMethod(i, ... );
      }
      return sum;
    }
};

A request to inline is merely a suggestion to the compiler, and anything more than a few operations on integral data types will probably not be inlined.  However, every file that has to use an inline method will also emit a function version in the resulting .o, even if the method was inlined. (This is to support function pointer behavior properly.)  Therefore, by requesting an inline in this case, you're likely just adding crud to the .o files which the linker will need to do work to resolve.

If the method has significant implementation, there's also a good chance that by not inlining it, you could eliminate some includes.

Stop inlining virtual methods.

You can't inline virtual methods under most circumstances, even if the method would otherwise be inlined because it's very short. The compiler must do runtime dispatch on any virtual method where the compiler doesn't know the object's complete type, which rules out the majority of cases where you have an object.

Stop inlining constructors and destructors.

Constructors and destructors are often significantly more complex than you think they are, especially if your class has any non-POD data members. Many STL classes have inlined constructors/destructors which may be copied into your function body. Because the bodies of these appear to be empty, they often seem like trivial functions that can safely be inlined.  Don't give in to this temptation.  Define them in the implementation file unless you really need them to be inlined.  Even if they do nothing now, someone could later add something seemingly-trivial to the class and make your hundreds of inlined destructors much more complex.

Even worse, inlining constructors/destructors prevents you from using forward declared variables:

class Forward;
class WontCompile {
  public:
    // THIS WON'T COMPILE, BUT IT WOULD HAVE IF WE PUT THESE IN THE
    // IMPLEMENTATION FILE!
    //
    // The compiler needs the definition of Forward to call the
    // vector/scoped_ptr ctors/dtors.
    Example() { }
    ~Example() { }

  private:
    std::vector<Forward> examples_;
    scoped_ptr<Forward> super_example_;
};

For more information, read Item 30 in Effective C++.

When you CAN inline constructors and destructors.

C++ has the concept of a trivial destructor. If your class has only POD types and does not explicitly declare a destructor, then the compiler will not bother to generate or run a destructor.

struct Data {
  Data() : count_one(0), count_two(0) {}
  // No explicit destructor, thus no implicit destructor either.

  // The members must all be POD for this trick to work.
  int count_one;
  int count_two;
};

In this example, since there is no inheritance and only a few POD members, the constructor will be only a few trivial integer operations, and thus OK to inline.

For abstract base classes with no members, it's safe to define the (trivial) destructor inline:

class Interface {
  public:
   virtual ~Interface() {}
  
   virtual void DoSomething(int parameter) = 0;
   virtual int GetAValue() = 0; 
};

But be careful; these two "interfaces" don't count:

class ClaimsToBeAnInterface : public base:RefCounted<ClaimsToBeAnInterface> {
 public:
  virtual ~ClaimsToBeAnInterface() { /* But derives from a template! */ }
};

class HasARealMember {
 public:
  virtual void InterfaceMethod() = 0;
  virtual ~HasARealMember() {}

 protected:
  vector<string> some_data_;
};

If in doubt, don't rely on these sorts of exceptions.  Err on the side of not inlining.

Be careful about your accessors.

Not all accessors are light weight. Compare:

class Foo {
 public:
  int count() const { return count_; }

 private:
  int count_;
};

Here the accessor is trivial and safe to inline.  But the following code is probably not, even though it also looks simple:

struct MyData {
  vector<GURL> urls_;
  base::Time last_access_;
};

class Manager {
 public:
  MyData get_data() { return my_data_; }

 private:
  MyData my_data_;
};

The underlying copy constructor calls for MyData are going to be complex. (Also, they're going to be synthesized, which is bad.)

What about code outside of headers?

For classes declared in .cc files, there's no risk of bloating several .o files with the definitions of the same "inlined" function.  However, it's still wise to follow the above guidelines, for a few reasons:

  • The compiler may still ultimately generate a smaller binary.
  • Your class may later be refactored to place the declaration into a .h file, at which point problematic inlines have a larger impact.
  • Being in a consistent habit everywhere reduces the risk of making mistakes, and results in more consistent code.
  • For many classes, having a separate declaration and definition makes it easier to see what the "class API" is, to better understand e.g. which base class functions the class overrides or what functionality it makes available.
For testing code, most of these factors are reduced or eliminated.  Test framework classes don't tend to be instantiated separately and passed around as objects; they're effectively just bundles of file-scope functionality coupled with a mechanism to reset state between tests.  In these cases, defining the test functions inline at their declaration sites has little negative effect and can reduce the amount of "boilerplate" in the test file.  Ultimately, test file inlining should be a readability-focused judgment call on the part of the test author and reviewer.

Static variables

Dynamic initialization of function-scope static variables is thread-unsafe in chromium, even on compilers/platforms such as gcc/linux where such idioms are normally safe.  Background can be found in this thread.

void foo() {
    static int BAD_COUNT = ComputeTheCount();  // THREAD-UNSAFE
    static int GOOD_COUNT = 42;  // C++03 3.6.2 says this is done before dynamic initialization, so probably thread-safe.
}

The majority of Chrome code is intended to be single-threaded, where this presents no problem.  When in multi-threaded code, however, the right answer is usually to use a base::LazyInstance.
Comments