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

universal/resilient ofMin/Max #7940

Closed
artificiel opened this issue May 8, 2024 · 1 comment · Fixed by #7944
Closed

universal/resilient ofMin/Max #7940

artificiel opened this issue May 8, 2024 · 1 comment · Fixed by #7944

Comments

@artificiel
Copy link
Contributor

from #7918

the idea is to prevent conversion to float with ofMin/Max (and maybe applicable to other) for the benefit of end users that mix/match types (typically floats (as returned by many OF functions), double (as dotted-decimals are interpreted in C++ source code) and int, also as textual interpretations).

comparing different types is not evident, especially when unsigned is thrown in (I guess that's why std::min does not attempt it, but our use case is probably defined enough to tackle in a specific way).

the logic:

  • if the args are same time, simply pass to std::min (which accepts anything that operates on <)
  • if args are different, find the std::common_type, then
  • if one of the members is size_t or uint64_t, treat it differently (there is no common type that can hold both uint64_t and anything signed, for instance, and it's undefined behaviour (no throw)) but in our case we don't need to return as such a common type as in the context of getting the minimum, the "result" will not be in the high uint64_t range (implicit, as the other arg is not size_t, it will be smaller).
  • if signed vs unsigned, check if the signed is < 0 before attempting cast of the "other" to prevent overflow of negatives.
template<typename T>
T ofMin(const T & t1, const T & t2) const { return std::min(t1, t2); }

template<typename T, typename Q>
auto ofMin(const T & t, const Q & q) const {
	using CommonType = typename std::common_type<T, Q>::type;
	if constexpr ((std::is_same_v<T, uint64_t> or std::is_same_v<T, size_t>) && std::is_signed_v<Q>) {
		if (q < 0) {
			return q;
		} else {
			return std::min<Q>(static_cast<Q>(t), q);
		}
	} else if constexpr ((std::is_same_v<Q, uint64_t> or std::is_same_v<Q, size_t>) && std::is_signed_v<T>) {
		if (t < 0) {
			return t;
		} else {
			return std::min<T>(t, static_cast<T>(q));
		}
	} else if constexpr (std::is_signed_v<T> && std::is_unsigned_v<Q>) {
		if (t < 0 || q > static_cast<Q>(std::numeric_limits<T>::max())) {
			return static_cast<CommonType>(t);
		} else {
			return std::min(static_cast<CommonType>(t), static_cast<CommonType>(q));
		}
	} else if constexpr (std::is_signed_v<Q> && std::is_unsigned_v<T>){
		if (q < 0 || t > static_cast<T>(std::numeric_limits<Q>::max())) {
			return static_cast<CommonType>(q);
		} else {
			return std::min(static_cast<CommonType>(t), static_cast<CommonType>(q));
		}
	} else {
		return std::min(static_cast<CommonType>(t), static_cast<CommonType>(q));
	}
}
@ofTheo
Copy link
Member

ofTheo commented May 9, 2024

looks good to me!
thanks @artificiel

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 a pull request may close this issue.

2 participants