Use MOZ_FINAL instead of NS_FINAL_CLASS for not-to-be-inherited classes, and mark non-overridable methods as MOZ_FINAL

[Jeff Walden]

Historically we've had the NS_FINAL_CLASS macro to annotate classes which should never be inherited. This relied on static analysis beyond normal builds for correctness. C++11 adds a contextual "final" keyword for this, which is now encapsulated in the MOZ_FINAL macro:

#include "mozilla/Attributes.h"
class F MOZ_FINAL { };
class D : public F { }; // ERROR: F is final
class B { };
class D2 MOZ_FINAL : public B { };

The MOZ_FINAL macro supplants the old, gunky NS_FINAL_CLASS macro: simply place MOZ_FINAL after the name of the class to be marked. (Unnamed classes can't be marked as final; this is a C++11 limitation.) Switching does lose us the static analysis bits, but that's actually a *good* thing. It turns out those bits were buggy, and some classes marked as "final" were inherited! Clearly static analysis tests haven't been run in awhile. Given that to the best of my knowledge no one has reported bugs about this, it's clearly better to have something some compilers will actually check -- and on many developers' machines, at that (Clang 3.0, and gcc 4.7 support final, and MSVC++2005+ supports it under the C# name "sealed") -- than to have something no one ever checks. NS_FINAL_CLASS will be removed once I get a reduced testcase to report a Clang bug that prevents the last remaining non-static-analysis-test use from being switched.

C++11 also adds the final keyword to virtual function declarations. A virtual function marked with the final keyword can't be overridden, and compilers supporting the final keyword will treat an attempt to override as an error. The MOZ_FINAL macro does double duty here:

#include "mozilla/Attributes.h"
struct B
{
virtual void foo() MOZ_FINAL;
virtual void bar() MOZ_FINAL { }
virtual void baz() MOZ_FINAL = 0;
};

Both uses of MOZ_FINAL provide some conceptual benefits in terms of reducing complexity of understanding. They also may provide some small performance benefits. Calling a final method doesn't need to incur virtual call overhead to work; calling *any* function on a final class doesn't need to incur virtual call overhead. (Although exactly how fast things will get probably depends on the linker as well as the compiler.) Bjarne Stroustrup thinks such performance micromanagement is usually "misplaced fear"[0], and regarding final he (at least once) thought "there didn't seem to be a sufficient need"[1]. But I'll let you be the judge of that for your code. :-)

Anyway, start using MOZ_FINAL where it makes sense. More detail here if you want it:

http://whereswalden.com/2011/11/26/introducing-moz_final-prevent-inheriting-from-a-class-or-prevent-overriding-a-virtual-function/
https://hg.mozilla.org/integration/mozilla-inbound/file/b5782f6da550/mfbt/Attributes.h#l174

Jeff

0. http://www2.research.att.com/~bs/bs_faq2.html#no-derivation
1. http://www2.research.att.com/~bs/bs_faq2.html#final

[Neil]

Jeff Walden wrote:

> virtual void baz() MOZ_FINAL = 0;

What does this even mean?

--
Warning: May contain traces of nuts.

[Jeff Walden]

On 11/27/2011 08:12 AM, Neil wrote:
>> virtual void baz() MOZ_FINAL = 0;
>
> What does this even mean?

Heh.

It means exactly what it looks like -- a pure-virtual method, therefore on an abstract class, which can't be overridden in subclasses. Now, whether that would ever be *useful*.... I can't think of a situation where it would be. But maybe there is one, if you think creatively enough about it. Reeeaallyy creatively.

Jeff

[dbradley]

On Nov 27, 11:12 am, Neil <n...@parkwaycc.co.uk> wrote:
> Jeff Walden wrote:
> >     virtual void baz() MOZ_FINAL = 0;
>
> What does this even mean?

I wondered the same thing myself. I couldn't find whether it's legal C+
+ or not. But from my understanding, you'd essentially have an
abstract class that couldn't have a legal derived class.

David

[Stuart Cook]

There seems to be a missing underscore in MOZ_HAVE_CX11_FINAL on line 78
of mfbt/Attributes.h.

http://mxr.mozilla.org/mozilla-central/source/mfbt/Attributes.h#78

[Justin Lebar]

Indeed. Would you mind filing a bug?

> _______________________________________________
> dev-platform mailing list
> dev-pl...@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-platform
>

[Stuart Cook]

On 30/11/11 2:22 AM, Justin Lebar wrote:
> Indeed. Would you mind filing a bug?

I tried, but I couldn't find a suitable component to file it under.

[Justin Lebar]

Yeah, I don't think we have a component for mfbt.

But if you file it under "core --> general" and cc me (jlebar), that
should work.

[Jeff Walden]

Yeah, use Core::General as Justin says -- and CC me too, I'll fix it (along with another typo I just noticed skimming this stuff again; I hate not-perfect compilers and macros).

There have been a spate of bugs on stuff for mfbt lately from me, but it's not clear to me whether the general level of bug-filing outside of this warrants a component.

Jeff

[Jeff Walden]

On 12/01/2011 11:45 AM, Jeff Walden wrote:
> Yeah, use Core::General as Justin says

...or actually, don't bother, I just fixed it:

http://hg.mozilla.org/integration/mozilla-inbound/rev/d896a4fb99f7

Thanks for pointing it out, probably no one would have noticed the omission otherwise. I still have no idea how you saw this. :-)

Jeff

[Stuart Cook]

On 2/12/11 1:28 PM, Jeff Walden wrote:
> Thanks for pointing it out, probably no one would have noticed the
> omission otherwise. I still have no idea how you saw this. :-)

It actually stands out a little more in MXR, because the MOZ_HAVE part
is a cross-reference link but the CXX11_FINAL part is not. Still a lucky
find, though.

(The fact that it's hard to spot is what really motivated me to say
something, since I knew that it would otherwise drive somebody bananas
in a few years' time.)