Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Fix for issue# 8158. #612

Merged
merged 1 commit into from Jul 1, 2012

Conversation

Projects
None yet
2 participants
Member

jmdavis commented May 29, 2012

MinType assumed that std.traits.mostNegative would work with the type that it was given, but mostNegative was designed to work with numeric types, not user-defined types.

@klickverbot klickverbot commented on the diff May 29, 2012

std/algorithm.d
@@ -5053,17 +5058,20 @@ Returns the minimum of the passed-in values. The type of the result is
computed by using $(XREF traits, CommonType).
*/
MinType!(T1, T2, T) min(T1, T2, T...)(T1 a, T2 b, T xs)
+ if(is(typeof(a < b)))
@klickverbot

klickverbot May 29, 2012

Member

If I wanted to pick nits, I might say that this should read b < a, but I guess due to opCmp it's okay either way.

@jmdavis

jmdavis May 29, 2012

Member

Yeah. For a moment, I was surprised that min and max didn't just use <, then I remembered that opCmp guarantees that that you always have both (unlike C++). So, checking for <, makes checking for >, <=, and >= always unnecessary. So, I just checked < and didn't worry about the ordering.

@klickverbot klickverbot commented on the diff May 29, 2012

std/algorithm.d
else
- alias T[0] MinType;
+ {
+ enum hasMostNegative = is(typeof(mostNegative!(T[0]))) &&
+ is(typeof(mostNegative!(T[1])));
@klickverbot

klickverbot May 29, 2012

Member

In theory, this could still not be enough if a user-defined type had a min property with a type different than what expected, but I'm not sure if this is worth considering (should probably be fixed in mostNegative then anyway).

@jmdavis

jmdavis May 29, 2012

Member

mostNegative won't even compile with user-defined types in almost all cases, because it checks whether T.min == 0 (hence the compilation error with user-defined types and min). Also, given its description, I think that it's pretty clear that mostNegative was never intended to work with user-defined types, which is why pull# 611 adds if(isNumeric!T || isSomeChar!T) as a template constraint on it.

@klickverbot

klickverbot May 29, 2012

Member

Ah, sorry, forgot that you added a constraint on it.

Member

jmdavis commented Jun 30, 2012

Any further comments or concerns before I merge this?

Member

klickverbot commented Jun 30, 2012

I don't think I found any issues when looking through the code back then, but I don't really remember, to be honest. Should be fine to merge, on a quick glance.

jmdavis added a commit that referenced this pull request Jul 1, 2012

@jmdavis jmdavis merged commit 6048caf into dlang:master Jul 1, 2012

Member

jmdavis commented Jul 1, 2012

Merged.

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