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

CRTP matchers #1554

Closed
wants to merge 3 commits into from
Closed

CRTP matchers #1554

wants to merge 3 commits into from

Conversation

dvirtz
Copy link
Contributor

@dvirtz dvirtz commented Feb 27, 2019

Description

As discussed in #1553, the current matchers implementation forces passing values by const ref. Changing the implementation to be CRTP based instead of dynamic polymorphism, enables forwarding the checked value as it is.
A nice benefit is that it is no longer required to explicitly set the value type for the Predicate matcher.

GitHub Issues

#1553

@codecov
Copy link

codecov bot commented Feb 28, 2019

Codecov Report

Merging #1554 into master will decrease coverage by 0.2%.
The diff coverage is 30%.

@@            Coverage Diff            @@
##           master    #1554     +/-   ##
=========================================
- Coverage   80.54%   80.33%   -0.2%     
=========================================
  Files         121      118      -3     
  Lines        3386     3361     -25     
=========================================
- Hits         2727     2700     -27     
- Misses        659      661      +2

@codecov
Copy link

codecov bot commented Feb 28, 2019

Codecov Report

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

@@            Coverage Diff             @@
##           dev-v3    #1554      +/-   ##
==========================================
+ Coverage   89.28%   94.29%   +5.01%     
==========================================
  Files         139        8     -131     
  Lines        5997       70    -5927     
==========================================
- Hits         5354       66    -5288     
+ Misses        643        4     -639

@dvirtz dvirtz force-pushed the pr.generic.matchers branch from 8575039 to 9c83cf5 Compare December 29, 2019 12:49
@dvirtz dvirtz changed the base branch from master to dev-v3 December 29, 2019 12:51
@dvirtz dvirtz force-pushed the pr.generic.matchers branch 3 times, most recently from 9ed5849 to e2f2134 Compare February 9, 2020 19:45
@dvirtz dvirtz force-pushed the pr.generic.matchers branch from e2f2134 to 6cf1e3d Compare February 9, 2020 22:12
@dvirtz
Copy link
Contributor Author

dvirtz commented Feb 9, 2020

I've renamed the new generic matcher to MatcherBaseGeneric and restored MatcherBase for keeping backward compatibility.
I also added more tests taken from #1843 (thanks @melak47).
All the custom matchers are implemented with MatcherBaseGeneric which should give them better performance as no virtual functions are used.

@melak47
Copy link
Contributor

melak47 commented Feb 9, 2020

In my PR I found I could get away without CRTP, just having a base class from the namespace where the non-member operator overloads are defined is enough to get them picked up via ADL.

(With a non-CRTP MatcherBase I think you could replace the existing virtual machinery with very little to no breaking changes. Only users who explicitly used override in their custom matchers would get an error. I don't know if this would be an acceptable amount of breakage)

horenmar added a commit that referenced this pull request Feb 16, 2020
This commit extends the Matchers feature with the ability to have type-independent (e.g. templated) matchers. This is done by adding a new base type that Matchers can extend, `MatcherGenericBase`, and overloads of operators `!`, `&&` and `||` that handle matchers extending `MatcherGenericBase` in a special manner.

These new matchers can also take their arguments as values and non-const references.

Closes #1307 
Closes #1553 
Closes #1554 

Co-authored-by: Martin Hořeňovský <martin.horenovsky@gmail.com>
@horenmar horenmar closed this Feb 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants