Googletest: Use override keyword with new C++11 compilers

[BillyDonahue]

_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 [email protected] on 15 Dec 2012 at 9:23

Attachments:

• GMock-use-override.patch

_Copied from original issue: google/googlemock#157_

[BillyDonahue]

_From @GoogleCodeExporter on August 24, 2015 22:40_

Original comment by [email protected] on 8 Mar 2013 at 5:42

• Changed state: Accepted

[BillyDonahue]

_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 [email protected] on 12 Nov 2013 at 1:37

[BillyDonahue]

_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 [email protected] on 5 Sep 2014 at 2:16

[BillyDonahue]

_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 [email protected] on 6 Sep 2014 at 5:38

[BillyDonahue]

_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 [email protected] on 6 Sep 2014 at 9:39

[BillyDonahue]

_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 [email protected] on 7 Sep 2014 at 4:46

[BillyDonahue]

_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 [email protected] on 8 Sep 2014 at 1:01

Attachments:

• Gmock-override.patch

[BillyDonahue]

_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 [email protected] on 12 Oct 2014 at 3:08

[BillyDonahue]

_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 [email protected] on 13 Oct 2014 at 5:39

[BillyDonahue]

_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 [email protected] on 14 Oct 2014 at 7:58

[BillyDonahue]

_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 [email protected] on 14 Oct 2014 at 11:44

[BillyDonahue]

_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 [email protected] on 16 Oct 2014 at 3:54

[BillyDonahue]

_From @GoogleCodeExporter on August 24, 2015 22:40_

Original comment by [email protected] on 31 Oct 2014 at 11:55

[BillyDonahue]

_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 [email protected] on 27 Nov 2014 at 7:05

[BillyDonahue]

_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 [email protected] on 27 Nov 2014 at 11:22

[Galabar001]

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

[Zitrax]

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

[ThalmicRevane]

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]

It's been discussed here in the past, with the arguments against it laid
out. can't search right now.

On Thu, Feb 23, 2017 at 10:49 AM Edwin Vane [e-mail]
wrote:

I agree. How has this argument been going on for 1.5 years? Does anybody
actually maintain this project anymore?

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
https://github.com/google/googletest/issues/533#issuecomment-282030120,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AINnROlvX6rB_6dfeFgdO2z96rNtZdYHks5rfaqigaJpZM4FyJy-
.

[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.

[SkyToGround]

So, yet another 9 months have passed. Is there any progress here?

[ArekPiekarz]

Seeing as there are no updates on this issue, I can suggest using Trompeloeil mocking framework. It supports modern function specifiers like override and noexcept out of the box.

[CoranH]

Still hoping to see override implemented in some form. Even as additional macros to maintain backwards compatibility. This feature is so crazily useful, as it's such a common and confusing problem when an interface changes (even slightly, such as const), and then the behaviour of the test shifts in an unpredictable/confusing manner.

[Bu11etmagnet]

I hacked "override" into all the MOCK_METHOD* macros in my local copy of gtest. The I had to define MOCK_METHOD[123]NOV without the "override" when I wanted to mock a method which was not in the base class.
I think there should be separate MOCK_METHOD macros with and without override.

[gennadiycivil]

I have been cleaning up older and inactive GitHub googletest issues. You may see an issue "closed" if it appears to be inactive/abandoned
Thank you

[gennadiycivil]

We have been slowly adding "override" as googletest requires C++11. At the same time this is not a priority .
If this presents a real issue we would gladly consider a PR - I would suggest running clang-modernize and extracting "override" output
https://clang.llvm.org/extra/clang-tidy/checks/modernize-use-override.html

[kwesolowski]

While finding good solution seems hard to make everyone happy, would just using:

_Pragma("clang diagnostic push") \
_Pragma("clang diagnostic ignored "-Winconsitent-missing-override"")
_Pragma("clang diagnostic pop")

be good enough?

[gennadiycivil]

@kwesolowski good solution is running clang-modernize and extracting "override" output
https://clang.llvm.org/extra/clang-tidy/checks/modernize-use-override.html.
We would welcome a PR to this effect

[Bu11etmagnet]

It's a bit difficult to run clang-modernize on pump input

[kwesolowski]

@gennadiycivil this can be used actual violations in gtest code, but override cannot fix MOCK_METHOD macros because they BY DESIGN allow all of:
overriding
shadowing
introducing new methods

And for user of GMOCK those macros are main painpoint, becouse other missing overrides are easy to avoid by -isystem.

[gennadiycivil]

The scope of this issue is "Use override keyword with new C++11 compilers". This has been opened since 2015 for what looks to me as internal housekeeping. ( Well before my time).

Since then the discussion has been all over the place with regards to the actual scope without any actionable PRs.

I am closing it for clarity. If there is an actual actionable issue , preferably with a PR to look at , it will be welcomed.