Use override keyword with new C++11 compilers

[GoogleCodeExporter]

The C++11 standard introduces new "override" and "final" keywords in virtual 
function declarations. The "override" keyword is especially useful in context 
of creating mock functions. Macros MOCK_METHOD* expand to virtual methods that 
have to be present in base class in order to be useful as mocks. And, since we 
have to define mocks and class interface separately, this creates a possible 
gap for bugs when changing method prototypes.

For example, changing a type of argument in base class method, without updating 
the associated MOCK_METHOD* macro, will define the mock method as overloading 
and compilers will silently allow such buggy code to compile. However, using 
the new C++11 feature, mismatched function declarations will result in compile 
error, thus allowing the programmer to clearly state his purpose.

In my projects, I patched GMock macros to optionally include the override 
keyword - see the attached diff. I think such improvement merits for inclusion 
in upstream, so I decided to create this issue.

Original issue reported on code.google.com by piot...@gmail.com on 15 Dec 2012 at 9:23

Attachments:

• GMock-use-override.patch

[GoogleCodeExporter]

Original comment by w...@google.com on 8 Mar 2013 at 5:42

• Changed state: Accepted
• Added labels: ****
• Removed labels: ****

[GoogleCodeExporter]

has this been fixed? just spend a few hours searching for a bug because there 
was no override keyword present :(

Original comment by alex.lan...@googlemail.com on 12 Nov 2013 at 1:37

• Added labels: ****
• Removed labels: ****

[GoogleCodeExporter]

Beware that this might not be a good thing to do, because the original 
statement:

"Macros MOCK_METHOD* expand to virtual methods that have to be present in base 
class in order to be useful as mocks"

is flawed, because there are situations where a mock method is not present in 
the mocked base class and still can be useful.

In the fact some of techniques described in the GoogleMock cookbook rely on 
creating mock methods which are not present in the original interface (base 
class), for example:
https://code.google.com/p/googlemock/wiki/CookBook#Simplifying_the_Interface_wit
hout_Breaking_Existing_Code
https://code.google.com/p/googlemock/wiki/CookBook#Mocking_Destructors

Original comment by Emil.Mas...@gmail.com on 5 Sep 2014 at 2:16

• Added labels: ****
• Removed labels: ****

[GoogleCodeExporter]

Well, I think that such cases are rare. In other 99% cases we want to make sure 
the method signatures match. I would propose to have the default MOCK_METHOD* 
macros use override, while another set of macros like FAKE_MOCK_METHOD* could 
be used in cases where we do not override any existing method.

And BTW, this issue is 1.5 years old, and seems to be ignored or forgotten :(

Original comment by piot...@gmail.com on 6 Sep 2014 at 5:38

• Added labels: ****
• Removed labels: ****

[GoogleCodeExporter]

The issue isn't forgotten, but let's discuss it if you like. I'd love to get 
the safety of override, but I'm concerned that non-overriding MOCK_METHOD are 
not that rare. Maybe a transition would work.

  1) Introduce new NONOVERRIDE_MOCK_METHOD* (or other bikesheddable name) macros, These expand into a function def marked 'virtual'. Introduce an opt-in build flag to enable MOCK_METHOD* to be 'override', defaulted false.

  2) At some point, flip that flag and make MOCK_METHOD* into 'override' by default in C++11. Everybody will have had a while to transition their unusual macros to the new name.

Original comment by billydon...@google.com on 6 Sep 2014 at 9:39

• Added labels: ****
• Removed labels: ****

[GoogleCodeExporter]

Well either way is fine for me. The point is to introduce such feature in 
official gmock release, so that I and others don't have to maintain our own 
patches.

Original comment by piot...@gmail.com on 7 Sep 2014 at 4:46

• Added labels: ****
• Removed labels: ****

[GoogleCodeExporter]

Billydon... That is almost what we did - we added the override with a new 
non-override macro but didn't add the word virtual (With a macro to turn 
on/off). I have attached my changes here. (Just note they are against R359 and 
done directly against the .h and not the pump file - I didn't notice the pump 
file when I did the change).

Original comment by arieh.sc...@gmail.com on 8 Sep 2014 at 1:01

• Added labels: ****
• Removed labels: ****

Attachments:

• Gmock-override.patch

[GoogleCodeExporter]

I believe that besides your GMOCK_USE_OVERRIDE def you should also take into 
account actual C++11 support, shouldn't you?

Original comment by stilew...@gmail.com on 12 Oct 2014 at 3:08

• Added labels: ****
• Removed labels: ****

[GoogleCodeExporter]

More specifically 'compiler' support, as Microsoft has had the override
keyword available for years.

Original comment by arieh.sc...@gmail.com on 13 Oct 2014 at 5:39

• Added labels: ****
• Removed labels: ****

[GoogleCodeExporter]

Please don't implement this as an incompatible, breaking change.  Use a new 
macro naming convention, e.g. MOCK_VMETHOD*, instead of changing the behavior 
of MOCK_METHOD* (and maybe use the varargs macro feature in C++11 to 
simplify?).  Personally, it would break the compilation of most of the tests 
I've written.  Non-virtual methods are the norm not the exception, e.g. there 
are only 6 public virtual functions in the Standard C++ library (see Herb 
Sutter's post: www.gotwa.ca/publications/mill18.htm).

Original comment by mh...@bluezoosoftware.com on 14 Oct 2014 at 7:58

• Added labels: ****
• Removed labels: ****

[GoogleCodeExporter]

"Non-virtual methods are the norm not the exception, e.g. there are only 6 
public virtual functions in the Standard C++ library (see Herb Sutter's post: 
www.gotwa.ca/publications/mill18.htm)."

Okay, most functions aren't virtual, but we only have to consider the functions 
being mocked. These should be virtual except in unusual circumstances like in:
https://code.google.com/p/googlemock/wiki/CookBook#Mocking_Nonvirtual_Methods

And I think those are long-tail users. I'd like to ask those users to change to 
MOCK_NONOVERRIDE_METHOD*, etc and leave the MOCK_METHOD* for the more 
mainstream cases.

As a data point, how long would you need to update the tests that are mocking 
nonvirtuals if we went forward?

Original comment by billydon...@google.com on 14 Oct 2014 at 11:44

• Added labels: ****
• Removed labels: ****

[GoogleCodeExporter]

"Okay, most functions aren't virtual, but we only have to consider the 
functions being mocked. These should be virtual except in unusual circumstances 
like in:
https://code.google.com/p/googlemock/wiki/CookBook#Mocking_Nonvirtual_Methods"

That's not unusual (that's my point).  It's even got a name: Non-Virtual 
Interface Idiom or NVI for short (again, see 
http://www.gotw.ca/publications/mill18.htm).

Using the technique you reference and others, e.g. conditionally substituting a 
mock class from a nested namespace via "use namespace" into the parent 
namespace of the original class, you can mock non-virtual methods that the unit 
under test may be calling to test all branches of the code.

Original comment by mh...@bluezoosoftware.com on 16 Oct 2014 at 3:54

• Added labels: ****
• Removed labels: ****

[GoogleCodeExporter]

Original comment by thakis@chromium.org on 31 Oct 2014 at 11:55

• Added labels: ****
• Removed labels: ****

[GoogleCodeExporter]

When deciding on how to handle this issue, you might want to consider how C++14 
reference qualifiers should be handled.

Original comment by mh...@bluezoosoftware.com on 27 Nov 2014 at 7:05

• Added labels: ****
• Removed labels: ****

[GoogleCodeExporter]

My proposed patch in issue 122 includes some support for reference qualifiers:
  MOCK_QUALIFIED_METHOD0(Name, &&, int());

But unfortunately:
  MOCK_QUALIFIED_METHOD0(Name, override, int());
Will not work, as the qualifiers are also applied to the gmock_$method magic... 
suggestions welcome ;)

Original comment by edgar.el...@caris.com on 27 Nov 2014 at 11:22

• Added labels: ****
• Removed labels: ****