MockFunction improvements #2319

adambadura · 2019-07-11T14:52:23Z

No description provided.

adambadura · 2019-07-11T15:18:57Z

It seems the change 3d70a6b (Add MockFunction::AsLambda (#2278)) will not build since it contains:

template <typename R, typename... Args>
class MockFunction<R(Args...)> {
  /* ... */

  auto AsLambda() {
    return [this](Args... args) -> R {
      return this->Call(std::forward<Args>(args)...);
    };
  }

  /* ... */
};

This form of auto return type deduction requires C++14. While apparently here we have only C++11.

If this cannot be changed (seems reasonable to not require too much) then the code either would have to use preprocessor for providing AsLambda with C++14 and higher only or would have to be dropped altogether. Which is fine as well, from my point of view the key parts are bb1c776 (Document MockFunction::AsStdFunction (#2265)) and cd3b9df (Add support for std::function in MockFunction (#2277)) anyway.

I was using CMake while developing. (I tried the Bazel but didn't understand it at all...) This means that probably CMake is under-configured by not being strict on the language standard as use. Otherwise, I would see the error immediately. If C++11 is indeed the required standard I will try to update CMake correspondingly.

adambadura · 2019-07-11T18:17:26Z

I was using CMake while developing. (I tried the Bazel but didn't understand it at all...) This means that probably CMake is under-configured by not being strict on the language standard as use. Otherwise, I would see the error immediately. If C++11 is indeed the required standard I will try to update CMake correspondingly.

This seems more complicated. The main CMakeLists.txt does include

if (CMAKE_VERSION VERSION_LESS "3.1")
  add_definitions(-std=c++11)
else()
  set(CMAKE_CXX_STANDARD 11)
  set(CMAKE_CXX_STANDARD_REQUIRED ON)
  if(NOT CYGWIN)
    set(CMAKE_CXX_EXTENSIONS OFF)
  endif()
endif()

It seems to be an issue between CMake and Visual C++, even though I have recent versions of both (3.14.0 and 19.21.27702.2 respectively) and this Visual C++ does support specifying language standard. Maybe the issue is that it does not support specifying C++11, only C++14 or higher.

The C++11 config does work with Cygwin.

gmock-spec-builders.h uses std::function (in MockFunction) but did not include <functional> to provide it. Apparently, it worked since the header must have been included by something else but better be safe than sorry.

Use of the = default instead of {} provides some advantages like proper constexpr or noexcept support. Furthermore, it keeps the type aggregate and trivial if it otherwise would be. MockFunction doesn’t truly benefit from those. However, it doesn’t also lose anything on using = default instead of {}. While it does add uniformity, follows guidelines (for example Chromium style guide), and protects against eager static checkers.

adambadura · 2019-07-11T22:48:27Z

I have dropped the problematic change 3d70a6b (Add MockFunction::AsLambda (#2278)). Instead, I have added a change updating CMake support under Cygwin.

kuzkry

Btw. recently there's been some rename of GoogleMock docs but your change should easily rebase onto it.

kuzkry · 2019-07-15T19:31:55Z

googlemock/include/gmock/gmock-spec-builders.h

+
+  std::function<R(Args...)> AsStdFunction() {
+    return [this](Args... args) -> R {
+      return this->Call(std::forward<Args>(args)...);


This and other std::forwards won't forward anything. args can never be variadic universal references here as they are not function template parameters. Imo, std::move works better here (as it will try to move, unless not possible (l-refs and nonmovable types)).

Firstly, note that I haven't written it so. I only moved it a bit. It was so before.

Secondly, I think you are not right about it anyway. This was originally std::move back in time when the class was still located in gmock-generated-function-mockers.h but was corrected to std::forward by 7796273 related to #1867.

Sorry then, thanks for a good reference to another PR.

gennadiycivil · 2019-07-15T20:03:07Z

Thank your for this contribution. Could you please elaborate and provide more information on exactly the issue / problems this is trying to address.
What is the before/after effect this would create and how is this beneficial?

adambadura · 2019-07-15T20:08:57Z

Btw. recently there's been some rename of GoogleMock docs but your change should easily rebase onto it.

Well, it was rebased over the HEAD until your changes got merged in. ;)

adambadura · 2019-07-15T21:07:48Z

Thank your for this contribution. Could you please elaborate and provide more information on exactly the issue / problems this is trying to address.
What is the before/after effect this would create and how is this beneficial?

Essentially there are three changes here:

Minor correction. (1st and 2nd commits)
Document currently undocumented MockFunction::AsStdFunction. (3rd commit)
Allow MockFunction to accept std::function (or other similar by user extension) as template argument and extract the function signature out of it. (4th commit)
Minor improvement. (5th commit)

kuzkry

Btw. recently there's been some rename of GoogleMock docs but your change should easily rebase onto it.

Well, it was rebased over the HEAD until your changes got merged in. ;)

It's only a kind conflict reminder of mine ;)

kuzkry · 2019-07-16T10:30:33Z

googlemock/include/gmock/gmock-spec-builders.h

+
+  std::function<R(Args...)> AsStdFunction() {
+    return [this](Args... args) -> R {
+      return this->Call(std::forward<Args>(args)...);


Sorry then, thanks for a good reference to another PR.

gennadiycivil · 2019-07-16T15:31:22Z

@adambadura Thank you for this request. There are too many things going on at the same time here.
I see documentation changes, CMake changes, code changes, etc.
I recommend splitting this into separate small PRs so we can take a proper look.
Thanks again

googlebot added the cla: yes label Jul 11, 2019

adambadura added 5 commits July 12, 2019 00:00

Update CMake for Cygwin

363f9be

Add missing <functional> include

93a9f91

gmock-spec-builders.h uses std::function (in MockFunction) but did not include <functional> to provide it. Apparently, it worked since the header must have been included by something else but better be safe than sorry.

Document MockFunction::AsStdFunction (google#2265)

b3bfa2f

Add support for std::function in MockFunction (google#2277)

a050743

adambadura force-pushed the MockFunction branch from 2451cf0 to bfaa7a4 Compare July 11, 2019 22:47

kuzkry reviewed Jul 15, 2019

View reviewed changes

kuzkry reviewed Jul 16, 2019

View reviewed changes

gennadiycivil closed this Jul 16, 2019

This was referenced Jul 16, 2019

Correct CMake to cover Cygwin #2331

Merged

Add missing <functional> include #2332

Merged

Document MockFunction::AsStdFunction (#2265) #2333

Closed

adambadura mentioned this pull request Jul 27, 2019

Add support for std::function in MockFunction (#2277) #2350

Merged

adambadura deleted the MockFunction branch August 3, 2019 10:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MockFunction improvements #2319

MockFunction improvements #2319

adambadura commented Jul 11, 2019

adambadura commented Jul 11, 2019 •

edited

Loading

adambadura commented Jul 11, 2019

adambadura commented Jul 11, 2019 •

edited

Loading

kuzkry left a comment

kuzkry Jul 15, 2019

adambadura Jul 15, 2019

kuzkry Jul 16, 2019

gennadiycivil commented Jul 15, 2019

adambadura commented Jul 15, 2019 •

edited

Loading

adambadura commented Jul 15, 2019

kuzkry left a comment

kuzkry Jul 16, 2019

gennadiycivil commented Jul 16, 2019

MockFunction improvements #2319

MockFunction improvements #2319

Conversation

adambadura commented Jul 11, 2019

adambadura commented Jul 11, 2019 • edited Loading

adambadura commented Jul 11, 2019

adambadura commented Jul 11, 2019 • edited Loading

kuzkry left a comment

Choose a reason for hiding this comment

kuzkry Jul 15, 2019

Choose a reason for hiding this comment

adambadura Jul 15, 2019

Choose a reason for hiding this comment

kuzkry Jul 16, 2019

Choose a reason for hiding this comment

gennadiycivil commented Jul 15, 2019

adambadura commented Jul 15, 2019 • edited Loading

adambadura commented Jul 15, 2019

kuzkry left a comment

Choose a reason for hiding this comment

kuzkry Jul 16, 2019

Choose a reason for hiding this comment

gennadiycivil commented Jul 16, 2019

adambadura commented Jul 11, 2019 •

edited

Loading

adambadura commented Jul 11, 2019 •

edited

Loading

adambadura commented Jul 15, 2019 •

edited

Loading