rollbear / trompeloeil

Header only C++14 mocking framework
Boost Software License 1.0
802 stars 85 forks source link

Matchers should receive non-const parameters when possible #331

Closed DNKpp closed 4 months ago

DNKpp commented 5 months ago

When working with matchers, I often stumble across a minor inconvenience when using matchers directly as a REQUIRE_CALL parameter. Given the following situation:

struct Mock
{
    MAKE_MOCK1(foo, void(int));
};

foo does not imply any restrictions on its param (it's a non-const value), but still any matchers receive it as const-ref => this custom matcher does not compile:

const auto matcher = trompeloeil::make_matcher<trompeloeil::wildcard>(
    [](int& value)
    {
 return value == 42;
    },
    [](std::ostream&)
    {
    });

As its usually good style to provide read-only arguments as const-ref, this has some implications what we can actually check. In my case its the ranges::any_view from range-v3 lib, which is actually unusable when constraint with const, as empty, begin and end members are not const overloaded. Nevertheless there is sometimes the necessity to apply matchers on non-const refs, which currently is not directly supported. As the placeholders (_1, ...) do not promote to const, I can of course check with an WITH statement, but that seems just like a workaround to me. The issue is, that you put each param into a tuple of references, but then pass the tuple around as const-ref, which then is supplied into std::get, which actually does the const promotion on the elements. That's a pitty, but instead of creating a tuple of lvalue-refs, we can switch to a tuple of std::reference_wrapper and unwrap those where necessary.

DNKpp commented 5 months ago

As a side-note: Matching std::reference_wrapper params doesn't even compile on current msvc, so this should not break any existing code. I tested this before my changes.

struct Mock
{
    MAKE_MOCK1(foo, void(std::reference_wrapper<int>));
};

TEST_CASE_METHOD(
  Fixture,
  "C++11: Matchers work with std::reference_wrapper params.",
  "[C++11][C++14][matching]")
{
  const auto matcher = trompeloeil::make_matcher<trompeloeil::wildcard>(
    [](const std::reference_wrapper<int>& value)
    {
      return value.get() == 42;
    },
    [](std::ostream&)
    {
    });

  {
    Mock u;
    REQUIRE_CALL_V(u, foo(matcher));
    int x = 42;
    u.foo(x);
  }
  REQUIRE(reports.empty());
}
codecov[bot] commented 5 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 97.47%. Comparing base (45583ab) to head (98dc713). Report is 6 commits behind head on main.

:exclamation: Current head 98dc713 differs from pull request most recent head c8255d8. Consider uploading reports for the commit c8255d8 to get more accurate results

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #331 +/- ## ========================================== - Coverage 99.31% 97.47% -1.84% ========================================== Files 12 11 -1 Lines 1022 951 -71 Branches 22 21 -1 ========================================== - Hits 1015 927 -88 Misses 7 7 - Partials 0 17 +17 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

rollbear commented 4 months ago

Nice. Thanks. I usually don't comment on code formatting, but that one line stands out as a bit weird. I'll merge as soon as you have pushed a fix for it.

DNKpp commented 4 months ago

Sry, but which line are you talking about?^^ I have a very hard time trying to stick with the 2 space indentation, so all of the formatting is done manually, but might be messed up by resharper.

rollbear commented 4 months ago

u.func_lr(x); is indented 6 spaces, the lines above it are indented 2 spaces.

DNKpp commented 4 months ago

Oh, well. That was a tab :) /done

rollbear commented 4 months ago

Excellent. Thank you so much!