C++ Dos and Don'ts

Contents

1 Minimize Code in Headers
2 Stop inlining code
3 Static variables

Minimize Code in Headers

Don't include headers you don't need. Especially in headers.

I've seen several places where a header will include something, like <deque> or "googleurl/src/gurl.h" and then not use the std::deque or GURL in the file. These are probably oversights from previous refactorings or reorganizations where the files used to be necessary, but they should be fixed whenever they are seen.

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_;
};

Whenever you can forward declare, you should forward declare instead of including a header.

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 headers 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 a handful of 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 that have hard to forward declare types, too.

Stop inlining code

BACKGROUND: We're doing too much inlining. Remember that definitions in classes are requested to be inlined. Remember that asking the compiler to inline a method is a suggestion, one that's often ignored by the compiler. A compiler that never inlined anything would be perfectly standards compliant. 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 behaviour properly.)

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 ComputerSomehting() {
   int sum =0;
   for (int i = 0; i < shurg; ++i) {
   sum += OtherMethod(i, ... );
   }
   return sum;
   }
};

Anything more than a few operations on integral data types will probably not be inlined by the compiler. Remember that inline is a suggestion, and that you're just adding crud to the .o files which the linker will need to do work to resolve.

If the method deals with implementation details, there's also a good chance that you could eliminate some included headers in the header.

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.

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

Even worse though, 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_;
};

When you CAN inline constructors and destructors.

The above case had two class members and that's a large part of the reason it's more complex than it seems. There are cases where it is better to have an inline constructor and no destructor.

struct Data {
  Data() : count_one(0), count_two(0) {}

  int count_one;
  int count_two;
};

C++ has the concept of a trivial destructor. In the above case, no destructor will be run because it only contains integral data types and the destructor is implicit. The constructor (since it has no base class and only integral data members) will be two trivial integer operations, which is OK to inline.

If you aren't sure, than you probably shouldn't try this.

You CAN inline virtual destructors in pure abstract base classes.

For example, you don't have to make a CC file for this:

class Interface {
public:
virtual ~Interface() {}

virtual void DoSomething(int parameter) = 0;
virtual int GetAValue() = 0;
};

But note that it really needs to be an abstract base class. 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_;
};

Think about your accessors.

Not all accessors are light weight. Compare:

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

private:
int count_;
};

Which is a simple compared to the simple looking:

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

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

private:
  MyData my_data_;
};

This is complex because the underlying copy constructor calls are going to be complex. (Also, they're going to be synthesized, which is bad.)

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.

Static variables

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.

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

The Chromium Projects

Quick links

Other sites