The Chromium Projects

Search this site

Except as otherwise noted, the content of this page is licensed under a Creative Commons Attribution 2.5 license, and examples are licensed under the BSD License.
For Developers‎ > ‎Coding Style‎ > ‎

Chromium Style Checker Errors

Introduction

Due to constant over inlining, we now have plugins to the clang compiler which check for certain patterns that can increase code bloat. This document lists these errors, explains the motivation behind adding it as an error and shows how to fix them. The chromium/clang wiki page explains how you can run the plugin yourself, or how you can run your CLs through the clang trybots which run this plugin as well.

All diagnostics produced by the style checker are marked with [chromium-style].

Constructor/Destructor Errors

Constructors and destructors are often much larger and more complex than programmers think. They are also implicitly defined in each translation unit if you don't declare them. This can have a cascading effect if a class has member variables that have inlined/implicitly declared constructors/destructors, or are templates.

(Internally, the plugin determines whether a class is complex using a point system, adding 9 points for each templated base class, 10 for each templated member variable, 3 for each non-POD type member variable, and (only for the constructor score) 1 for each integral data type. If a class's score is greater than or equal to 10, it will trip these errors.)

If you get any of these compiler errors:
  • Complex class/struct needs an explicit out-of-line constructor
  • Complex class/struct needs an explicit out-of-line destructor
It's because you've written something like the following in a header:

class ComplexStuff {
 public:
  void NoDeclaredConstructor();

 private:
  // Enough data members to trip the detector
  BigObject big_object_;
  std::map<std::string, OtherObj> complex_stl_stuff_;
};

This is fixed by adding declared constructors:

class ComplexStuff {
 public:
  // Defined in the cc file:
  ComplexStuff();
  ~ComplexStuff();

 private:
  BigObject big_object_;
  std::map<std::string, OtherObj> complex_stl_stuff_;
};

Likewise, if you get these compiler errors:
  • Complex constructor has an inlined body.
  • Complex destructor has an inline body.
It's because you've written something like the following in a header:

class MoreComplexStuff {
 public:
  MoreComplexStuff() {}
  ~MoreComplexStuff() {}

 private:
  BigObject big_object_;
  std::map<std::string, OtherObj> complex_stl_stuff_;
};

The solution is the same; put the definitions for the constructor/destructor in the implementation file.

Virtual Method Out-of-lining

Virtual methods are almost never inlined in practice. Because of this, methods on a base class will be emitted in each translation unit that allocates the object or any subclasses that don't override that method. This is usually not a major gain, unless you have a wide class hierarchy in which case it can be a big win (http://codereview.chromium.org/5741001/). Since virtual methods will almost never be inlined anyway (because they'll need to go through vtable dispatch), there are almost no downsides.

If you get the error:
  • virtual methods with non-empty bodies shouldn't be declared inline
It's because you wrote something like this:

class BaseClass {
 public:
  virtual void ThisOneIsOK() {}
  virtual bool ButWeDrawTheLineAtMethodsWithRealImplementation() { return false; }
};

And can be fixed by out of lining the method that does work:

class BaseClass {
 public:
  virtual void ThisIsHandledByTheCompiler() {}
  virtual bool AndThisOneIsDefinedInTheImplementation();
};

Missing "virtual" keywords

Unlike the previous problems, which can affect debug binary size, this one is purely a style problem, with a simple solution.

If you get the error:
  • Overridden method must have "virtual" keyword
It is because given this base class:

class Interface {
 public:
   virtual void OurMethod() = 0;

 protected:
  virtual ~Interface() {}
};

You then created a derived class that misses the "virtual" keyword like this:

class Derived : public Interface {
 public:
  void OurMethod();
};

You can fix this easily:

class Derived : public Interface {
 public:
  virtual void OurMethod();
};

Overriding without OVERRIDE

If you get the error:
  • Overriding method must be marked with OVERRIDE.
It is because you are overriding a base class method, yet not annotating that override with OVERRIDE. For example:

class BaseClass {
 public:
  virtual ~BaseClass() {}
  virtual void SomeMethod() = 0;
};

class DerivedClass : public BaseClass {
 public:
  virtual void SomeMethod();
};

The solution? Just tell the compiler what you intend:

#include "base/compiler_specific.h"

[...]

class DerivedClass : public BaseClass {
 public:
  virtual void SomeMethod() OVERRIDE;
};

This allows the compiler to warn you when your intentions stop matching parsed reality.

"Using namespace" in a header

You should never have using namespace directives in a header because this can change the lookup rules in unrelated implementation files.
This is enforced by clang's -Wheader-hygiene warning. This isn't checked by the style plugin.

If you get the error:
  • using namespace directive in global context in header [-Wheader-hygiene]
It's because you did something like this in a header:

using namespace WebKit;

class OurDerivedClass : public WebKitType {
};

You can fix this by removing the "using namespace" line and explicitly stating the namespace:

class OurDerivedClass : public WebKit::WebKitType {
};

RefCounted types with public destructors

When you declare a type to be referenced counted by making it derive from base::RefCounted or base::RefCountedThreadSafe, you're indicating that it should not be destructed until the last reference is Released, either explicitly or by destructing a scoped_refptr<>. Having a public destructor allows this contract to be violated, by allowing callers to stack allocate the object, store it in a scoped_ptr<> instead of a scoped_refptr<>, or to explicitly delete the object before all references have gone away. Because this can lead to subtle security bugs, the style checker will warn if class that is derived from a RefCounted type has either an explicit public destructor or an implicit destructor, which is automatically public.

If you get the error:
  • Classes that are ref-counted should not have public destructors.
It's because you did something like this:

class Foo : public base::RefCounted<Foo> {
 public:
  Foo() {}
  ~Foo() {}
};

You can fix this by ensuring that the destructor is either protected (if expecting classes to subclass this base) or private. You'll also need to ensure that the RefCounted type is friended, as appropriate.

class Foo : public base::RefCounted<Foo> {
 public:
  Foo() {}

 private:
  friend class base::RefCounted<Foo>;
  ~Foo() {}
};

If you get the error:
  • Classes that are ref-counted should have explicit destructors that are protected or private.
It's because you did something like this:

class Foo : public base::RefCounted<Foo> {
  // Compiler generated constructors, destructor, and copy operator are public.
};

You can fix it by ensuring you explicitly declare a protected or private destructor:

class Foo : public base::RefCounted<Foo> {
 // Compiler generated constructors and copy operator are public.
 private:
  friend class base::RefCounted<Foo>;
  ~Foo() {}  // Explicit destructor is private.
};
Comments