702437 - Implement MOZ_OVERRIDE to start using C++11 override controls

Status: RESOLVED FIXED

(Core :: MFBT, defect)

Description

[Jeff Walden [:Waldo]]

Attachment: Patch

In a fit of boredom yesterday I implemented MOZ_OVERRIDE, a macro to encapsulate the C++11 contextual 'override' keyword.

If you alter a virtual member function's signature in a base class, generally you have to change overriding instances of that member function in derived classes.  If you don't, your overrides will no longer override, and things can get screwy.  C++11's contextual override keyword helps avoid this by requiring that a virtual member function override one found on a base class:

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

Were Base::f() changed to take an argument, or its name changed to Base::f2(), all the "overrides" would be compile errors.  This basically solves the problem of changes to base-class virtual member functions requiring updates to derived-class implementations.

Gunky XPCOM code has an NS_OVERRIDE annotation for this, which expands to a user attribute under gcc-like compilers.  It's only used by static analysis, so its value is limited.  Unfortunately its position is different -- necessarily so, because in the C++11 override keyword position it attaches to the return value of the method:

  class Derived1 : public Base
  {
    public:
      NS_OVERRIDE virtual void f();
  };
  class Derived2 : public Base
  {
    public:
      /* attribute annotates f()'s return value */
      virtual void f() __attribute__(...);
  };

For now I'm just adding MOZ_OVERRIDE without touching NS_OVERRIDE.  (I could have added MOZ_OVERRIDE to existing NS_OVERRIDE users but decided I don't quite have the time now.  I'll keep it as a chore for a future time when my mind's gone too fuzzy to do something else productive.)  It's possible MOZ_OVERRIDE renders NS_OVERRIDE irrelevant, but I don't know how the NS_OVERRIDE attribute gets used, so I couldn't say for sure.  In the meantime compiler support that doesn't require a static-analysis build seems more valuable than keeping instantiations of the concept to an absolute minimum.

Clang 3+ supports this, gcc 4.7+ (i.e. their bleeding-edge trunk only) supports this, and perhaps most interestingly MSVC++ 2005 and later supports this (bleed-over from the same keyword being in C#, apparently).  So support's not universal, but we're only a gcc release away from it working pretty much everywhere we care about.

Comment 1

[Jeff Walden [:Waldo]]

Attachment: Oops, forgot to qref

Comment 2

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Is |override| visible through GIMPLE to our analysis scripts in gcc 4.7?  If so, we can just do away with NS_OVERRIDE in favor of MOZ_OVERRIDE.

Comment 3

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

>diff --git a/gfx/layers/basic/BasicLayers.cpp b/gfx/layers/basic/BasicLayers.cpp

>   NS_OVERRIDE virtual already_AddRefed<gfxASurface>
>-  CreateBuffer(Buffer::ContentType aType, const nsIntSize& aSize);
>+  CreateBuffer(Buffer::ContentType aType, const nsIntSize& aSize) MOZ_OVERRIDE;
> 
>   void DestroyBackBuffer()
>   {

I'm OK with having MOZ_OVERRIDE coexist in m-c with NS_OVERRIDE during
this transitionary period, like Homo sapiens coexisted with Homo
neanderthalensis, but having them in the same file is likely to cause
trouble, like Cro-Magnon and Neanderthal trying to share a cave.
Let's not go there.

>diff --git a/mfbt/Types.h b/mfbt/Types.h
>--- a/mfbt/Types.h
>+++ b/mfbt/Types.h
>@@ -91,6 +91,15 @@
> #ifdef __cplusplus
> 
> /*
>+ * A general note: g++ requires -std=c++0x or -std=gnu++0x to support various
>+ * C++11 functionality without warnings (functionality used by the macros
>+ * implemented below).  These modes are detectable by checking whether
>+ * __GXX_EXPERIMENTAL_CXX0X__ is defined or, more standardly, by checking
>+ * whether __cplusplus has a C++11 or greater value.  Current versions of g++ do
>+ * not correctly set __cplusplus, so we check both for forward compatibility.
>+ */
>+
>+/*
>  * MOZ_DELETE, specified immediately prior to the ';' terminating an undefined-
>  * method declaration, attempts to delete that method from the corresponding
>  * class.  An attempt to use the method will always produce an error *at compile
>@@ -115,13 +124,6 @@
> #if defined(__clang__) && (__clang_major__ >= 3 || (__clang_major__ == 2 && __clang_minor__ >= 9))
> # define MOZ_DELETE            = delete
> #elif defined(__GNUC__) && (__GNUC__ > 4 || (__GNUC__ == 4 && __GNUC_MINOR__ >= 4))
>- /*
>-  * g++ >= 4.4 requires -std=c++0x or -std=gnu++0x to support deleted functions
>-  * without warnings.  These modes are detectable by the experimental macro used
>-  * below or, more standardly, by checking whether __cplusplus has a C++11 or
>-  * greater value.  Current versions of g++ do not correctly set __cplusplus, so
>-  * we check both for forward compatibility.
>-  */
> # if defined(__GXX_EXPERIMENTAL_CXX0X__) || __cplusplus >= 201103L
> #  define MOZ_DELETE           = delete
> # else
>@@ -131,6 +133,64 @@
> # define MOZ_DELETE            /* no = delete support, or unknown support */
> #endif
> 

In addition to refactoring this comment, I think we're at the point
now where it makes sense to refactor the compiler-detection gunk, too.
If we pull all this stuff under the general comment above and use
"feature macros" in the helper decls, I think the code will be shorter
on the whole, and easier to read where the MOZ_* helpers are declared.
E.g.

#if defined(__clang__)
# if __clang_major__ >= 3
# define HAVE_CXX1X_OVERRIDE
# if (__clang_major__ >= 3 || (__clang_major__ == 2 && __clang_minor__ >= 9))
#  define HAVE_CXX1X_DELETE
# endif
#elif defined(__GNUC__)
  ...

#if HAVE_CXX1X_DELETE
#  define MOZ_DELETE           = delete
#else
#  define MOZ_DELETE
#endif

 ...

Would like to see the next version.

Comment 4

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #2)
> Is |override| visible through GIMPLE to our analysis scripts in gcc 4.7?  If
> so, we can just do away with NS_OVERRIDE in favor of MOZ_OVERRIDE.

Actually, disregard this question, it's not relevant.

Let's file a followup to nuke NS_OVERRIDE.

Comment 5

[Joshua Cranmer [:jcranmer]]

There also exists NS_FINAL as well, which we may be interested in.

Comment 6

[Jeff Walden [:Waldo]]

Yeah, I expect we'll have MOZ_FINAL in addition to the rest of these at some point.  Knowing we have override-like functionality and users of it already, that seemed like a better place to start.

Comment 7

[Jeff Walden [:Waldo]]

Attachment: v2

Maybe someone else finds one-space indentation in #ifdefs understandable, but I find it horribly difficult to distinguish if-levels with it: it's just not enough indentation difference.  Any chance we could switch to two-space indent in a separate patch?

Comment 8

[Jeff Walden [:Waldo]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d88e310d5781

I filed bug 702877 for removing NS_OVERRIDE.

I'd also still like feedback on comment 7, for what it's worth.

Comment 9

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Oh sorry.  Sure, two-space indent there is fine with me.

Comment 10

[Jeff Walden [:Waldo]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/94b2b53b2db6 for two-space indentation.

Comment 11

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/94b2b53b2db6

Comment 12

[Ed Morley [:emorley]]

And the original landing:
https://hg.mozilla.org/mozilla-central/rev/d88e310d5781

Comment 13

[Jeff Walden [:Waldo]]

Blog post, also noting the deprecation of NS_OVERRIDE:

http://whereswalden.com/2011/11/16/introducing-moz_override-to-annotate-virtual-functions-which-override-base-class-virtual-functions/

And m.d.platform mail:

https://groups.google.com/group/mozilla.dev.platform/browse_thread/thread/343f2140635bc81f#

Comment 14

[Jeff Walden [:Waldo]]

I consider the comments by these macros to be much better documentation than anything maintained externally, so I think this document roughly pointing people to it is reasonable documentation:

https://developer.mozilla.org/en/mfbt

Note that this macro no longer lives in Util.h, rather in Attributes.h now, so that people have a better chance guessing where it's defined in mfbt/ based solely on the functionality desired.