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 custom implementation of min() and max() with std::min() and std::max() #188

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

MoonPadUSer
Copy link
Contributor

This PR removes the custom implementation of min() and max() and replaces it with std::min() and std::max()

@robalni
Copy link
Contributor

robalni commented Aug 2, 2020

Why was this done? What is the reason to use min and max from std?

@MoonPadUSer
Copy link
Contributor Author

yes, we wanna get rid of most custom written functions and types and switch to using their standard alternatives. that way we might be able to get rid of geom.h and tools.h some day

@robalni
Copy link
Contributor

robalni commented Aug 2, 2020

Why do we not want to have geom.h and tools.h?

@TheAssassin
Copy link
Member

TheAssassin commented Aug 2, 2020

Many of the things written in tools.h behave in strange ways, hide memory problems or behave unexpectedly otherwise. Also, they're redundant to the STL. And they lack the necessary interface so you cannot use STL algorithms with them. They don't support modern C++ features like e.g., rvalue references. There is no point in having implemented everything by yourself. The STL is well tested and known to work, also most C++ developers know how to use it.

Also, most headers with these custom template classes are e.g., included more than once, among with other weirdnesses. I've never seen code have to include headers more than once without having header guards or #pragma once the way it's done here.

@voidanix
Copy link
Member

voidanix commented Jul 9, 2022

@MoonPadUSer can you please rebase and squash both this and #189?

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

Successfully merging this pull request may close these issues.

4 participants