Last Comment Bug 702437 - Implement MOZ_OVERRIDE to start using C++11 override controls
: Implement MOZ_OVERRIDE to start using C++11 override controls
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: MFBT (show other bugs)
: Trunk
: All All
: -- minor (vote)
: mozilla11
Assigned To: Jeff Walden [:Waldo] (remove +bmo to email)
:
:
Mentors:
Depends on:
Blocks: 702877
  Show dependency treegraph
 
Reported: 2011-11-14 14:25 PST by Jeff Walden [:Waldo] (remove +bmo to email)
Modified: 2012-02-01 13:59 PST (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (14.02 KB, patch)
2011-11-14 14:25 PST, Jeff Walden [:Waldo] (remove +bmo to email)
no flags Details | Diff | Splinter Review
Oops, forgot to qref (14.04 KB, patch)
2011-11-14 15:11 PST, Jeff Walden [:Waldo] (remove +bmo to email)
no flags Details | Diff | Splinter Review
v2 (14.71 KB, patch)
2011-11-15 09:35 PST, Jeff Walden [:Waldo] (remove +bmo to email)
cjones.bugs: review+
Details | Diff | Splinter Review

Description Jeff Walden [:Waldo] (remove +bmo to email) 2011-11-14 14:25:24 PST
Created attachment 574424 [details] [diff] [review]
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] (remove +bmo to email) 2011-11-14 15:11:54 PST
Created attachment 574443 [details] [diff] [review]
Oops, forgot to qref
Comment 2 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-11-14 21:49:54 PST
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 2011-11-14 22:01:15 PST
>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 2011-11-14 22:01:51 PST
(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] 2011-11-15 06:57:59 PST
There also exists NS_FINAL as well, which we may be interested in.
Comment 6 Jeff Walden [:Waldo] (remove +bmo to email) 2011-11-15 08:52:09 PST
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] (remove +bmo to email) 2011-11-15 09:35:15 PST
Created attachment 574608 [details] [diff] [review]
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] (remove +bmo to email) 2011-11-15 21:30:57 PST
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 2011-11-15 21:56:15 PST
Oh sorry.  Sure, two-space indent there is fine with me.
Comment 10 Jeff Walden [:Waldo] (remove +bmo to email) 2011-11-15 22:28:53 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/94b2b53b2db6 for two-space indentation.
Comment 11 Ed Morley [:emorley] 2011-11-16 03:09:09 PST
https://hg.mozilla.org/mozilla-central/rev/94b2b53b2db6
Comment 12 Ed Morley [:emorley] 2011-11-16 03:10:20 PST
And the original landing:
https://hg.mozilla.org/mozilla-central/rev/d88e310d5781
Comment 14 Jeff Walden [:Waldo] (remove +bmo to email) 2011-12-22 14:18:19 PST
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.

Note You need to log in before you can comment on or make changes to this bug.