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

Replace std::binary_function with std::function #31

Merged
merged 2 commits into from
Aug 25, 2017

Conversation

iondune
Copy link
Contributor

@iondune iondune commented Aug 3, 2017

std::binary_function is deprecated in C++11, removed in C++17 - replace with std::function

@brofield
Copy link
Owner

brofield commented Aug 3, 2017

I'm happy to merge something like this, but I suspect that std::function may still not be supported in older libraries or other platforms. What have you tested this with?

@Chocobo1
Copy link
Contributor

std::function is added since C++11.
So there should be a #if checking for older C++ version.

@zhmu
Copy link

zhmu commented Aug 24, 2017

Note that all that std::binary_function<> does is add a few typedef, which do not seem to be used in this code. Thus, the inheritance can safely be removed.

Note that std::function<> is a different beast, and I would not use it here (it does not add anything): all the compiler cares about is that it can call operator(), which is the case. So please, let's not make things harder than they need to be.

Should support for C++11 become mandatory in this project, I think a lambda-function would be a better way forward (but I am having the feeling @brofield does not want to drop support for pre-C++11 and that is something we should respect)

@iondune
Copy link
Contributor Author

iondune commented Aug 24, 2017

I have updated the PR to simply remove the inheritance from these structs - as I understand it, this will remove deprecation warnings on C++17 compilers while preserving pre-C++11 compatibility.

@brofield brofield merged commit b414bb2 into brofield:master Aug 25, 2017
@brofield
Copy link
Owner

Thanks for your contribution. Merged now.

@jlblancoc jlblancoc mentioned this pull request Oct 4, 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.

4 participants