Use override keyword with new C++11 compilers #533

Open
BillyDonahue opened this Issue Aug 25, 2015 · 20 comments

Projects

None yet

4 participants

@BillyDonahue
Collaborator

From @GoogleCodeExporter on August 24, 2015 22:40

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:

Copied from original issue: google/googlemock#157

@BillyDonahue
Collaborator

From @GoogleCodeExporter on August 24, 2015 22:40

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

  • Changed state: Accepted
@BillyDonahue
Collaborator

From @GoogleCodeExporter on August 24, 2015 22:40

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

@BillyDonahue
Collaborator

From @GoogleCodeExporter on August 24, 2015 22:40

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

@BillyDonahue
Collaborator

From @GoogleCodeExporter on August 24, 2015 22:40

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

@BillyDonahue
Collaborator

From @GoogleCodeExporter on August 24, 2015 22:40

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

@BillyDonahue
Collaborator

From @GoogleCodeExporter on August 24, 2015 22:40

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

@BillyDonahue
Collaborator

From @GoogleCodeExporter on August 24, 2015 22:40

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

Attachments:

@BillyDonahue
Collaborator

From @GoogleCodeExporter on August 24, 2015 22:40

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

@BillyDonahue
Collaborator

From @GoogleCodeExporter on August 24, 2015 22:40

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

@BillyDonahue
Collaborator

From @GoogleCodeExporter on August 24, 2015 22:40

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

@BillyDonahue
Collaborator

From @GoogleCodeExporter on August 24, 2015 22:40


"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

@BillyDonahue
Collaborator

From @GoogleCodeExporter on August 24, 2015 22:40

"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

@BillyDonahue
Collaborator

From @GoogleCodeExporter on August 24, 2015 22:40

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

@BillyDonahue
Collaborator

From @GoogleCodeExporter on August 24, 2015 22:40

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

@BillyDonahue
Collaborator

From @GoogleCodeExporter on August 24, 2015 22:40

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

@murraycu murraycu added a commit to murraycu/murrayc-tuple-utils that referenced this issue Sep 23, 2016
@murraycu murraycu Fix the build with --enable-warnings=fatal.
By not using -Wsuggest-override (with warnings as errors), though it
is wonderfully useful, because googletest doesn't use the override
keyword yet:
google/googletest#533
de88060
@Galabar001

Just got hit by this. A couple of hours wasted. :(

@murraycu murraycu added a commit to murraycu/murrayc-tuple-utils that referenced this issue Dec 9, 2016
@murraycu murraycu Build: Use a separate set of warnings when building googletest.
Because it doesn't use the override keyword:
google/googletest#533
7d0e9ef
@murraycu murraycu added a commit to murraycu/murrayc-tuple-utils that referenced this issue Dec 9, 2016
@murraycu murraycu Build: Tests: Use -isystem, instead of -I, for the googletest headers.
The compiler is then more forgiving of the code in the headers,
so we won't see a warning about the missing override keyword
( google/googletest#533 )
but we will still see warnings from our own code.
See https://gcc.gnu.org/onlinedocs/cpp/System-Headers.html

Thanks to Martin:
http://www.murrayc.com/permalink/2016/12/08/trying-googletest/#comment-332886
6e6c3ff
@murraycu murraycu added a commit to murraycu/prefixsuffix that referenced this issue Dec 9, 2016
@murraycu murraycu Build: Tests: Use separate warnings when building googletest.
And use the googletest headers via -isystem instead of -I.

The compiler is then more forgiving of the code in the headers,
so we won't see a warning about the missing override keyword
( google/googletest#533 )
but we will still see warnings from our own code.
See https://gcc.gnu.org/onlinedocs/cpp/System-Headers.html

Thanks to Martin:
http://www.murrayc.com/permalink/2016/12/08/trying-googletest/#comment-332886
888fd93
@Zitrax
Zitrax commented Feb 22, 2017

Frequent annoyance :(, it would be really nice to have this ability.

@ThalmicRevane
ThalmicRevane commented Feb 23, 2017 edited

I agree. How has this argument been going on for 1.5 years? Does anybody actually maintain this project anymore? Are there more active forks? Maybe ones that support C++11 and beyond?

@BillyDonahue
Collaborator
@ThalmicRevane

I see. Glad to know the project is still alive at least. I'll read through all the related issues. It definitely would be nice to have a solution of some sort.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment