Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature: generic matchers #1843

Merged
merged 18 commits into from
Feb 16, 2020
Merged

Feature: generic matchers #1843

merged 18 commits into from
Feb 16, 2020

Conversation

melak47
Copy link
Contributor

@melak47 melak47 commented Jan 25, 2020

Description

Currently, composable matchers are based on MatcherBase<T>, like EqualsMatcher<T>.
EqualsMatcher<T> can compare two std::vector<T>s for equality, but not a std::vector and a std::array, for example.
This can make writing tests cumbersome somtimes, as you either need to write your own matcher
for whatever containers you need to work with, or convert everything to std::vector.

This PR aims to implement support for matchers that can be generic in their argument types, while also:

  • not breaking compatibility with existing matchers
  • allowing combining generic and non-generic matchers via &&, ||, !
  • (ideally) not impacting compile time of code that only uses non-generic matchers

Here is a motivating example:

TEST_CASE("example") {
    std::array<int, 3> container{1,2,3};

    std::array<int, 3> a{1,2,3};
    std::vector<int> b{0,1,2};
    std::list<int> c{4,5,6};

    CHECK_THAT(container, EqualsRange(a) || EqualsRange(b) || EqualsRange(c));
}

While it's easy to write your own custom EqualsRange matcher, making it composable with other matchers using &&, ||, ! is not, since that requires deriving from a shared MatcherBase<T>.

With the changes from this PR, an implementation of EqualsRange could look like this:

template<typename Range>
struct EqualsRangeMatcher : Catch::MatcherGenericBase {

    EqualsRangeMatcher(Range const& range) : range{range} {}

    template<typename OtherRange>
    bool match(OtherRange const& other) const {
        using std::begin;
        using std::end;

        return std::equal(begin(range), end(range), begin(other), end(other));
    }

    std::string describe() const override {
        return "Equals: " + Catch::rangeToString(range);
    }

private:
    Range const& range;
};

template<typename Range>
auto EqualsRange(const Range& range) -> EqualsRangeMatcher<Range> {
    return EqualsRangeMatcher<Range>{range};
}

@codecov
Copy link

codecov bot commented Jan 26, 2020

Codecov Report

Merging #1843 into dev-v3 will increase coverage by 0.12%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           dev-v3    #1843      +/-   ##
==========================================
+ Coverage   89.12%   89.24%   +0.12%     
==========================================
  Files         142      144       +2     
  Lines        6037     6133      +96     
==========================================
+ Hits         5380     5473      +93     
- Misses        657      660       +3     

@horenmar
Copy link
Member

I fixed it up a bit.

EvilMatcher const* operator& () const {
throw EvilAddressOfOperatorUsed();
}
};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about operator,? 😈

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think anything here is vulnerable to that right now, but it can't hurt :)

TEST_CASE("Overloaded address-of operator is not used", "[matchers][templated]") {
REQUIRE_NOTHROW(EvilMatcher() || EvilMatcher());
REQUIRE_NOTHROW(EvilMatcher() && EvilMatcher());
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would add ! just to be thorough.

@horenmar
Copy link
Member

horenmar commented Feb 7, 2020

Cool, thanks.

@dvirtz dvirtz mentioned this pull request Feb 9, 2020
#include <string>
#include <type_traits>
#include <utility>
#include <vector>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This include should not be needed (I am also not sure what is used for, but I did not look hard)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I forgot to remove the include.

melak47 and others added 18 commits February 16, 2020 05:31
...and avoid copying their internal vectors.
Also check that new operator templates are not used with old matchers.
... and pass by reference to matcher, to allow matchers capturing by mutable reference
Move most of the implementation which doesn't depend on template
arguments to .cpp file.

Address review comments:
- use StringRef for combine argument
- calculate correct amount of space to reserve
remove index_sequence using declarations
replace last address-of operator use with std::addressof
@melak47
Copy link
Contributor Author

melak47 commented Feb 16, 2020

Sorry, rebased again.

@horenmar horenmar merged commit 17c4b2d into catchorg:dev-v3 Feb 16, 2020
@horenmar horenmar mentioned this pull request Feb 16, 2020
@melak47 melak47 deleted the dev-v3-generic-matchers branch February 16, 2020 19:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants