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

TestSuite::Comparator<T> now only requires copy ctor. #9

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
2 participants
@chris-chambers
Contributor

chris-chambers commented Jun 25, 2014

The default Comparator implementation previously required T to have a
parameterless constructor. Now it puts off construction until the
comparison is actually performed, and uses the copy constructor to
initialize the saved values.

TestSuite::Comparator<T> now only requires copy ctor.
The default Comparator<T> implementation previously required T to have a
parameterless constructor.  Now it puts off construction until the
comparison is actually performed, and uses the copy constructor to
initialize the saved values.
@chris-chambers

This comment has been minimized.

Show comment
Hide comment
@chris-chambers

chris-chambers Jun 25, 2014

Contributor

By the way, thanks for both Magnum and Corrade. They are both great!

Contributor

chris-chambers commented Jun 25, 2014

By the way, thanks for both Magnum and Corrade. They are both great!

@mosra

This comment has been minimized.

Show comment
Hide comment
@mosra

mosra Jun 26, 2014

Owner

Awesome, thanks!

Actually, looking at it, I think this could be done with std::optional as well, getting rid even of the last remaining allocation. I have a implementation of this class in Magnum repository, so I could move it here and use it.

I'm planning to do a release soon so I will postpone this change after that, if you don't mind.

Owner

mosra commented Jun 26, 2014

Awesome, thanks!

Actually, looking at it, I think this could be done with std::optional as well, getting rid even of the last remaining allocation. I have a implementation of this class in Magnum repository, so I could move it here and use it.

I'm planning to do a release soon so I will postpone this change after that, if you don't mind.

@chris-chambers

This comment has been minimized.

Show comment
Hide comment
@chris-chambers

chris-chambers Jun 27, 2014

Contributor

That's funny, I was just using std::optional for something else and realized it could be used here. I think that's a much better plan for the long-term.

Contributor

chris-chambers commented Jun 27, 2014

That's funny, I was just using std::optional for something else and realized it could be used here. I think that's a much better plan for the long-term.

@mosra

This comment has been minimized.

Show comment
Hide comment
@mosra

mosra Jul 9, 2014

Owner

After giving it some thought I made it even simpler, not making any restrictions to the type except for operator== and operator<<(Debug) (or now also operator<<(std::ostream&)). See commit cbed0c1.

Again thanks for the initial idea :-)

Owner

mosra commented Jul 9, 2014

After giving it some thought I made it even simpler, not making any restrictions to the type except for operator== and operator<<(Debug) (or now also operator<<(std::ostream&)). See commit cbed0c1.

Again thanks for the initial idea :-)

@mosra mosra closed this Jul 9, 2014

@chris-chambers

This comment has been minimized.

Show comment
Hide comment
@chris-chambers

chris-chambers Jul 9, 2014

Contributor

Oh, yes, this is a nice way to do it. Now I can delete the last of my local patches!

Contributor

chris-chambers commented Jul 9, 2014

Oh, yes, this is a nice way to do it. Now I can delete the last of my local patches!

@chris-chambers chris-chambers deleted the chris-chambers:test_suite_comparator_only_require_copy_ctor branch Jul 9, 2014

@mosra mosra added this to the 2015.05 milestone Feb 15, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment