26.11.11

Introducing MOZ_FINAL: prevent inheriting from a class, or prevent overriding a virtual function

Tags: , , , , , , , — Jeff @ 09:17

The inexorable march of progress continues in the Mozilla Framework Based on Templates with the addition of MOZ_FINAL, through which you can limit various forms of inheritance in C++.

Traditional C++ inheritance mostly can’t be controlled

In C++98 any class can be inherited. (An extremely obscure pattern will prevent this, but it has down sides.) Sometimes this makes sense: it’s natural to subclass an abstract List class as LinkedList, or LinkedList as CircularLinkedList. But sometimes this doesn’t make sense. StringBuilder certainly could inherit from Vector<char>, but doing so might expose many Vector methods that don’t belong in the StringBuilder concept. It would be more sensible for StringBuilder to contain a private Vector<char> which StringBuilder member methods manipulated. Preventing Vector from being used as a base class would be one way (not necessarily the best one) to avoid this conceptual error. But C++98 doesn’t let you easily do that.

Even when inheritance is desired, sometimes you don’t want completely-virtual methods. Sometimes you’d like a class to implement a virtual method (virtual perhaps because its base declared it so) which derived classes can’t override. Perhaps you want to rely on that method being implemented only by your base class, or perhaps you want it “fixed” as an optimization. Again in C++98, you can’t do this: public virtual functions are overridable.

C++11′s contextual final keyword

C++11 introduces a contextual final keyword for these purposes. To prevent a class from being inheritable, add final to its definition just after the class name (the class can’t be unnamed).

struct Base1 final
{
  virtual void foo();
};

// ERROR: can't inherit from B.
struct Derived1 : public Base1 { };

struct Base2 { };

// Derived classes can be final too.
struct Derived2 final : public Base2 { };

Similarly, a virtual member function can be marked as not overridable by placing the contextual final keyword at the end of its declaration, before a terminating semicolon, body, or = 0.

struct Base
{
  virtual void foo() final;
};

struct Derived : public Base
{
  // ERROR: Base::foo was final.
  virtual void foo() { }
};

Introducing MOZ_FINAL

mfbt now includes support for marking classes and virtual member functions as final using the MOZ_FINAL macro in mozilla/Attributes.h. Simply place it in the same position as final would occur in the C++11 syntax:

#include "mozilla/Attributes.h"

class Base
{
  public:
    virtual void foo();
};

class Derived final : public Base
{
  public:
    /*
     * MOZ_FINAL and MOZ_OVERRIDE are composable; as a matter of
     * style, they should appear in the order MOZ_FINAL MOZ_OVERRIDE,
     * not the other way around.
     */
    virtual void foo() MOZ_FINAL MOZ_OVERRIDE { }
    virtual void bar() MOZ_FINAL;
    virtual void baz() MOZ_FINAL = 0;
};

MOZ_FINAL expands to the C++11 syntax or its compiler-specific equivalent whenever possible, turning violations of final semantics into compile-time errors. The same compilers that usefully expand MOZ_OVERRIDE also usefully expand MOZ_FINAL, so misuse will be quickly noted.

One interesting use for MOZ_FINAL is to tell the compiler that one worrisome C++ trick sometimes isn’t. This is the virtual-methods-without-virtual-destructor trick. It’s used when a class must have virtual functions, but code doesn’t want to pay the price of destruction having virtual-call overhead.

class Base
{
  public:
    virtual void method() { }
    ~Base() { }
};

void destroy(Base* b)
{
  delete b; // may cause a warning
}

Some compilers warn when they instantiate a class with virtual methods but without a virtual destructor. Other compilers only emit this warning when a pointer to a class instance is deleted. The reason is that in C++, behavior is undefined if the static type of the instance being deleted isn’t the same as its runtime type and its destructor isn’t virtual. In other words, if ~Base() is non-virtual, destroy(new Base) is perfectly fine, but destroy(new DerivedFromBase) is not. The warning makes sense if destruction might miss a base class — but if the class is marked final, it never will! Clang silences its warning if the class is final, and I hope that MSVC will shortly do the same.

What about NS_FINAL_CLASS?

As with MOZ_OVERRIDE we had a gunky XPCOM static-analysis NS_FINAL_CLASS macro for final classes. (We had no equivalent for final methods.) NS_FINAL_CLASS too was misplaced as far as C++11 syntax was concerned, and it too has been deprecated. Almost all uses of NS_FINAL_CLASS have now been removed (the one remaining use I’ve left for the moment due to an apparent Clang bug I haven’t tracked down yet), and it shouldn’t be used.

(Side note: In replacing NS_FINAL_CLASS with MOZ_FINAL, I discovered that some of the existing annotations have been buggy for months! Clearly no one’s done static analysis builds in awhile. The moral of the story: compiler static analyses that happen for every single build are vastly superior to user static analyses that happen only in special builds.)

Summary

If you don’t want a class to be inheritable, add MOZ_FINAL to its definition after the class name. If you don’t want a virtual member function to be overridden in derived classes, add MOZ_FINAL at the end of its declaration. Some compilers will then enforce your wishes, and you can rely on these requirements rather than hope for the best.

16.11.11

Introducing MOZ_OVERRIDE to annotate virtual functions which override base-class virtual functions

Tags: , , , , , , , — Jeff @ 09:55

Overriding inherited virtual functions

One way C++ supports code reuse is through inheritance. One base class implements common functionality. Then other classes inherit from it, essentially copying functionality from it. These other classes can add their own new functionality, or, more powerfully, they can override the base class functionality.

class Base
{
  public:
    virtual const char* type() { return "Base"; }
};
class Derived : public Base
{
  public:
    virtual const char* type() { return "Derived"; }
};

Overriding base class functionality is simple. Keeping such overrides working correctly is sometimes harder. The problem is that the override relationship is implicit: if the override doesn’t exactly match the signature of the desired function in the base class, it may not work correctly.

class Base
{
  public:
    // Perhaps as part of an incomplete refactoring,
    // the base class's function changed its name.
    virtual const char* kind() { return "Base"; }
};
class DerivedIncorrectly : public Base
{
  public:
    virtual const char* type() { return "Derived"; }
};

// BAD: code expecting kind() to work and sometimes
// indicate Derived-ness no longer will.

Making the override relationship explicit

Some languages (Scala, C#, probably others) provide the ability to mark a derived class’s function as an override of an inherited function. C++98 included no such ability, but C++11 does, through the contextual override keyword. When override is used, that virtual member function must override one found on a base class. If it does not, it is a compile error.

class Base
{
  public:
    virtual const char* kind() { return "Base"; }
};
class DerivedIncorrectly : public Base
{
  public:
    // This will cause a compile error: there's no type()
    // method on Base that this overrides.
    virtual const char* type() override { return "Derived"; }

    // This will work as intended.
    virtual const char* kind() override { return "Derived"; }
};

Introducing MOZ_OVERRIDE

The Mozilla Framework Based on Templates now includes support for the C++11 contextual override keyword, encapsulated in the MOZ_OVERRIDE macro in mozilla/Types.hmozilla/Attributes.h. Simply place it at the end of the declaration of the relevant method, before any = 0 or method body, like so:

#include "mozilla/Types.h" // MOZ_OVERRIDE has since moved...
#include "mozilla/Attributes.h" // ...to here

class Base
{
  public:
    virtual void f() = 0;
};
class Derived1 : public Base
{
  public:
    virtual void f() MOZ_OVERRIDE;
};
class Derived2 : public Base
{
  public:
    virtual void f() MOZ_OVERRIDE = 0;
};
class Derived3 : public Base
{
  public:
    virtual void f() MOZ_OVERRIDE { }
};

MOZ_OVERRIDE will expand to use the C++11 construct in compilers which support it. Thus in such compilers misuse of MOZ_OVERRIDE is an error. Even better, some of the compilers used by tinderbox support override, so in many cases tinderbox will detect misuse. (Specifically, MSVC++ 2005 and later support it, so errors in cross-platform and Windows code won’t pass tinderbox . Much more recent versions of GCC and Clang support it as well, but these versions are too new for tinderbox to have picked them up yet — in the case of GCC too new to even have been released yet. :-) )

What about NS_OVERRIDE?

It turns out there’s already a macro annotation to indicate an override relationship: NS_OVERRIDE. This gunky XPCOM macro expands to a user attribute under gcc-like compilers. It’s only used by static analysis right now, so its value is limited. Unfortunately its position is different — necessarily so, because in the C++11 override position it would attach to the return value of the method:

class OldAndBustedDerived : public Base
{
  public:
    NS_OVERRIDE virtual void f(); // annotates the method
    __attribute__(...) virtual void g(); // its expansion
};
class Derived2 : public Base
{
  public:
    // But in the MOZ_OVERRIDE position, it would annotate
    // f()'s return value.
    virtual void f() __attribute__(...);
};

NS_OVERRIDE is now deprecated and should be replaced with MOZ_OVERRIDE. With a little work, static analysis with new-enough compilers can likely look for MOZ_OVERRIDE just as easily as for NS_OVERRIDE. And since MOZ_OVERRIDE works in non-static analysis builds, it’s arguably better in the majority of cases anyway. If you’re looking for an easy way to improve Mozilla code, changing NS_OVERRIDE uses to use MOZ_OVERRIDE would be a simple way to help.

Summary

If you’ve overridden an inherited virtual member function and you’re worried that that override might silently break at some point, annotate your override with MOZ_OVERRIDE. This will cause some compilers to enforce an override relationship, making it much less likely that your intended relationship will break.

09.11.11

Introducing MOZ_DELETE, a macro improving upon the deliberately-unimplemented method idiom

C++ default operators and the sole-ownership idiom

Often a C++ class will solely manage some value: for example, a GtkWindow* or a void* for malloc‘d memory. The class will then release ownership in its destructor as appropriate. It would be extremely problematic to release ownership multiple times — think security-vulnerability-problematic. C++ copy construction and default assignment exacerbate this issue, because C++ automatically generates these methods for all classes, even when the default behavior breaks sole-ownership. The C++98 idiom solving this is to privately declare a copy constructor and a default assignment operator, then never define them:

struct Struct
{
  private:
    Struct(const Struct& other);
    void operator=(const Struct& other);
};

Declaring the methods privately prevents any code but friends of Struct from calling them. And by never defining them, even such friends will cause a link-time error if they try.

Disabling the default operators in C++11

Once you’re familiar with this idiom it’s not too bad. But initially, it’s pretty unclear. And nothing prevents someone from actually defining these methods. (They could only be used by Struct or friends of Struct, to be sure, but for sufficiently complex code it’s possible someone might make a mistake.) C++11 improves upon this trick by introducing deleted function syntax:

struct Struct
{
  private: // no longer necessary, but doesn't hurt
    Struct(const Struct& other) = delete;
    void operator=(const Struct& other) = delete;
};

Deleted functions are effectively removed from name lookup: using, defining, or referring to a deleted function produces a compile error — far better than a link error or, even worse, no error at all.

= delete support in mfbt

The Mozilla Framework Based on Templates now includes support for declaring a function only to prevent its use (or use of an inherited version). The MOZ_DELETE macro encapsulates this support:

#include "mozilla/Types.h" // MOZ_DELETE has since moved...
#include "mozilla/Attributes.h" // ...to here

struct Struct
{
  private:
    Struct(const Struct& other) MOZ_DELETE;
    void operator=(const Struct& other) MOZ_DELETE;
};

MOZ_DELETE isn’t as readable or understandable as = delete, but it’s searchable, and the comment next to its definition will clarify matters. If the declarations are private, MOZ_DELETE is just as good as the traditional idiom, and in compilers supporting C++11 deleted functions it’s better.

Which compilers support C++11 deleted functions? I’m aware of GCC since 4.4, Clang since 2.9, and ICC since 12.0. Rightly, if unfortunately, you must specify -std=c++0x or similar to use deleted function syntax without causing a warning. For various reasons Mozilla can’t do that yet, so MOZ_DELETE only produces the C++11 syntax when compiling with Clang (where we can pass -Wno-c++0x-extensions to disable the warning). I’d love to see it use C++11 syntax in GCC and ICC as well, but I don’t have the time to solve the -std=c++0x problem now, or to figure out another workaround. I’ve filed bug 701183 for this problem — help there is much appreciated.

Summary

Use MOZ_DELETE when declaring any method you will intentionally not implement. It’ll work better, and produce better errors, in some compilers. Those compilers don’t include GCC or ICC yet, but with your help they could. Any takers?

Update, evening of November 10, 2011: I just landed further changes to make MOZ_DELETE use C++11 syntax with GCC when compiling with -std=c++0x (which we apparently do more often than I’d thought), so you should now get its goodness in GCC as well — most of the time. In some “exotic” situations we don’t compile anything with -std=c++0x, so you won’t get any benefit there. Also, the JavaScript engine is never compiled with it. So if you want this to work fully, everywhere, you should use Clang.

03.11.11

How I organize my Mozilla trees

Tags: , , , — Jeff @ 10:17

Using Mozilla trees more smartly

A month ago I got a new laptop, requiring me to migrate my Mozilla trees, patches, and related work from old laptop to new. My previous setup was the simplest, stupidest thing that could work: individual clones of different trees, no sharing among those trees, sometimes multiple clones of the same tree for substantial, independent patchwork I didn’t want to explicitly order. Others have tried smarter tricks in the past, and I decided to upgrade my setup.

A new setup

The new setup is essentially this:

  • I have one local clone of mozilla-inbound in ~/moz/.clean-base which I never develop against or build against, and never modify except by updating it.
  • Whenever I want a mozilla-inbound tree, I clone ~/moz/.clean-base. I change the default-push entry in the new clone to point to the original mozilla-inbound. (I don’t change the default entry; pulling is entirely local.)
  • If I want to push a patch, I pull and update ~/moz/.clean-base. Then I pull and update the local clone that has the patch I want to push. Then I finish my patch and push it. Because default-push points to the remote mozilla-inbound, hg push as usual does exactly what I want.

Advantages

This setup has many advantages:

  • Getting a new mozilla-inbound tree is fast. I never clone the remote mozilla-inbound tree, because I have it locally. It’s not modified by a patch queue where I’d have to temporarily checkpoint work, pop to clone, then reapply after.
  • Updating a working mozilla-inbound tree is fast. Pulling and updating are completely local with no network delay.
  • I only need to update from the remote mozilla-inbound once for new changes to be available for all local trees. Instead of separately updating my SpiderMonkey shell tree, updating my browser tree, and updating any other trees I’m using, at substantial cost in time, one pull in ~/moz/.clean-base benefits all trees.
  • My working trees substantially share storage with ~/moz/.clean-base.

Pitfalls, and workarounds

Of course any setup has down sides. I’ve noticed these so far:

  • Updating a working trees is a two-step process: first updating ~/moz/.clean-base, then updating the actual tree.
  • I’ll almost always lose a push race to mozilla-inbound. If my local working tree is perfectly up-to-date with my ~/moz/.clean-base, that’s generally not up-to-date with the remote tree, particularly as rebasing my patches is now a two-step process. That produces a larger window of time for others to push things after I’ve updated my clean tree but before I’ve rebased my working tree.
  • I have to remember to edit the default-push in new trees, lest I accidentally mutate ~/moz/.clean-base.

Some of these problems are indeed annoying, but I’ve found substantial workarounds for them such that I no longer consider them limitations.

Automate updating ~/moz/.clean-base

Updating is only a two-step process if I update ~/moz/.clean-base manually, but it’s easy to automate this with a cronjob. With frequent updates ~/moz/.clean-base is all but identical to the canonical mozilla-inbound. And by making updates automatic, I also lose push races much less frequently (particularly if I rebase and push right after a regular update).

I’ve added this line to my crontab using crontab -e to update ~/moz/.clean-base every twenty minutes from 07:00-01:00 every day but Sunday (this being when I might want an up-to-date tree):

*/20 00-01,07-23 * * 1-6 /home/jwalden/moz/inflight/pull-updated-inbound >/dev/null 2>&1

I perform the update in a script, piping all output to /dev/null so that cron won’t mail me the output after every update. It seems better to have a simpler crontab entry, so I put the actual commands in /home/jwalden/moz/inflight/pull-updated-inbound:

#!/bin/bash

cd ~/moz/.clean-base/
hg pull -u

With these changes in place, updating a working tree costs only the time required to rebase it: network delay doesn’t exist. And the intermediate tree doesn’t intrude on my normal workflow.

Add a hook to ~/moz/.clean-base to prevent inadvertent pushes

My setup depends on ~/moz/.clean-base being clean. Local changes or commits will break automatic updates and might corrupt my working trees. I want ~/moz/.clean-base to only change through pulls.

I can enforce this using a Mercurial prechangegroup hook. This hook, run when a repository is about to accept a group of changes, can gate changes before they’re added to a tree. I use such a hook to prevent any changes except by a push by adding these lines to ~/moz/.clean-base/.hg/hgrc:

# Prevent pushing into local mozilla-inbound clone: only push after changing a clone's default-push.
[hooks]
prechangegroup.prevent_pushes = python:prevent_pushes.prevent_pushes.hook

This invokes the hook function in prevent_pushes.py:

#!/usr/bin/python

def hook(ui, repo, **kwargs):
  source = kwargs['source']
  if source != 'pull':
    print "Changes pushed into non-writable repository!  Only pulls permitted."
    return 1
  print "Updating pristine mozilla-inbound copy..."
  return 0

On my Fedora-based system, I place this file in /usr/lib/python2.7/site-packages/prevent_pushes/ beside an empty __init__.py. Mercurial will find it and invoke the hook whenever ~/moz/.clean-base receives changesets.

Only pushing from a new clone without a default-push would attempt to modify ~/moz/.clean-base, so the need to prevent changes to ~/moz/.clean-base might seem small. Yet so far this hook has prevented such changes more than once when I’ve forgotten to set a default-push, and I expect it will again.

Conclusion

There are doubtless many good ways to organize Mozilla work. I find this system works well for me, and I hope this description of it provides ideas for others to incorporate into their own setups.