the Chromium logo

The Chromium Projects
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:

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:

It's because you've written something like the following in a header:

class MoreComplexStuff {
 public:
  MoreComplexStuff() = default;
  ~MoreComplexStuff() = default;
 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.

Sometimes, you might need to inline a constructor/destructor for performance reasons. The style checker makes an exception for explicitly inlined constructors/destructors:

In the .h file:

class ComplexFastStuff {
 public:
  ComplexFastStuff();
  ~ComplexFastStuff();
 private:
  /* complicated members */
};

In the .cc file:

inline ComplexFastStuff::ComplexFastStuff() = default;

inline ComplexFastStuff::~ComplexFastStuff() = default;

If this class is being copied somewhere in the code, then the compiler will generate an implicit copy constructor for the class. As a result, you might also see the following error:

This can be fixed by adding an explicit copy constructor to your class:

In the .h file:

class ComplexCopyableStuff {
  public:
   ComplexCopyableStuff();
   ComplexCopyableStuff(const ComplexCopyableStuff& other);
   ~ComplexCopyableStuff();
  private:
   /* complicated members */
};

In the .cc file:

ComplexCopyableStuff::ComplexCopyableStuff(const ComplexCopyableStuff& other) = default;

Note that by defaulting the implementation, you ensure that you don't forget to copy a member and this code will not need to be updated as the members of the class change.

Also note that copying a complex class can be an expensive operation. The preferred solution is almost always is to avoid the copy if possible. Please consider changing the code and eliminating unnecessary copies instead of simply adding an out of line copy constructor to silence the error. Additionally, in a large number of cases, you can utilize move semantics to improve the efficiency of the code.

Usually a compiler would generate a move constructor for your class, making moving objects efficient. This is particularly true for STL containers such as std::map. However, specifying either a copy constructor or a destructor prevents the compiler from generating a move constructor. All in all, you should prefer specifying a move constructor instead of a copy constructor. In cases where your object is used as an rvalue, this will also prevent the error since the compiler will not generate a copy constructor:

In the .h file:

class ComplexMovableStuff {
 public:
  ComplexMovableStuff();
  ComplexMovableStuff(ComplexMovableStuff&& other);
  ~ComplexMovableStuff();
 private:
  /* complicated members */
};

In the .cc file:

ComplexMovableStuff::ComplexMovableStuff(ComplexMovableStuff&& other) = default;

For more information on move semantics, please refer to https://www.chromium.org/rvalue-references

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:

It's because you wrote something like this:

class BaseClass {
 public:
  virtual void ThisOneIsOK() = default;
  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();
};

Virtual methods and override/final

If you get the error:

It is because you are overriding a base class method, yet not annotating that override with override or final. 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:

// ...
class DerivedClass : public BaseClass {
 public:
  void SomeMethod() override;
};

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

Redundant Virtual specifiers

If you get one of the following errors:

Just remove the redundant specifier. Google style is to use only one of virtual, override, or final per method. For example:

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

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

Can be simply written:

// ...
class DerivedClass : public BaseClass {
 public:
  void SomeMethod() final;
};

Virtual final base class methods

If you get the error:

The virtual method does not override anything and is final; consider making it non-virtual

That means you have a final method that's not overriding anything. In that case, consider removing the virtual keyword, since no one can override the method anyway. For example:

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

Should be written:

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

"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:

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:

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:

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

Enumerator max values

If you have an enumerator value that needs to be the max value (useful for histograms or for IPC), declare your enum as an enum class and name the max value kMaxValue. The plugin will automatically warn if kMaxValue is no longer the max value.

For example:

enum class Foo {
  kOne,
  kTwo,
  kMaxValue = kOne,
};

Will produce the error:

This can be fixed by assigning the correct value to kMaxValue:

enum class Foo {
  kOne,
  kTwo,
  kMaxValue = kTwo,
};