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

Math functions Min/Max #84

Merged
merged 1 commit into from
Dec 10, 2017
Merged

Math functions Min/Max #84

merged 1 commit into from
Dec 10, 2017

Conversation

Ragnar-F
Copy link
Contributor

@Ragnar-F Ragnar-F commented Oct 11, 2017

These functions and their Transform counterparts are used to determine the lowest or highest value of several numeric input values. While Transform only supports two input values, the functions accept more than two one or more input values (up to 10000 if all parameters are explicitly specified, or quasi-infinite if an array is passed as a variadic parameter). If one the input values is non-numeric, a blank value is returned. The return value inherits the number type (integer or float) of the highest/lowest input value.

To the best of my knowledge, I have tried to keep the code performant and small. Perhaps it can still be improved. There should be no breaking changes, since already existing functions of the same name overwrite the built-in ones.

I think it is a great convenience for the user to use these functions instead of using the ternary operator (or a loop for more than two values).

If you are interested in merging, I will write the corresponding documentation.

Examples:

MsgBox, % Min(-2, 2.11) ; -2
MsgBox, % Max([1, 2, 3]*) ; 3
MsgBox, % Max("abc", 1) ; blank value
; Transform, test, Min, 1.324, 100
; MsgBox, % test ; 1.324000
; Transform, test, Max, -43, % False
; MsgBox, % test ; 0

@HelgeffegleH
Copy link
Contributor

Nice 👍

@a-raccoon
Copy link

I'm always happy to see a function added 10 years late. Better than never! 🍆

@fincs
Copy link
Contributor

fincs commented Oct 23, 2017

Was it really necessary to add a Transform subcommand?

@Ragnar-F
Copy link
Contributor Author

No, I only did this for the sake of completeness.

@HelgeffegleH
Copy link
Contributor

No, I only did this for the sake of completeness.

You could have made if for v2 😞 😁
But I guess that

BIF_DECL(BIF_MinMax)
{
// ...
}

will be usable for v2 also, so the main work is done 👍
Cheers.

@Lexikos
Copy link
Collaborator

Lexikos commented Oct 27, 2017

I would be more inclined to merge this if the Transform sub-commands were removed. There is no sense in adding code and documentation to extend a deprecated command.

@Ragnar-F
Copy link
Contributor Author

I've removed the commit for adding the Transform sub-commands.

@mmikeww
Copy link

mmikeww commented Nov 17, 2017

Should 1 parameter be allowed, and return the same value?

@joedf
Copy link
Member

joedf commented Nov 17, 2017

Why not? 👍

@Lexikos
Copy link
Collaborator

Lexikos commented Nov 18, 2017

This is why not: to assist error detection. Min(x) is more likely to be an error than legitimate (e.g. min(x y), where the comma was accidentally omitted). However, for min(array_of_values*) it makes sense to allow an array with only one item.

@Ragnar-F
Copy link
Contributor Author

I have reduced the parameter minimum to 1.

Besides the fact that I don't know how to use the current code to provide a different minimum parameter handling between the variadic and explicit variants (for example whether the first parameter is variadic or an array), I think the minimum number of parameters for both variants should be the same to avoid confusion. Since min(array_of_values*) seems to be more useful, I decided in favor of the variadic variant.

@Lexikos
Copy link
Collaborator

Lexikos commented Nov 21, 2017

That is acceptable.

Variadic parameters are expanded before the function is called. It is not feasible to differentiate between the two cases.

@Lexikos
Copy link
Collaborator

Lexikos commented Nov 26, 2017

The typecast to double causes loss of precision (and counter-intuitive results) when dealing with integers. The functions are incapable of differentiating between certain integers, with the result instead depending on the order of parameters:

MsgBox % Max(9223372036854775806, 9223372036854775807)
MsgBox % Max(9223372036854775807, 9223372036854775806)
MsgBox % Min(-9223372036854775807, -9223372036854775808)
MsgBox % Min(-9223372036854775808, -9223372036854775807)

Given the current behaviour of math operators with mixed types, it would make sense to treat all inputs as floating-point and produce a floating-point result if any one input is floating-point. (By contrast, I'd guess that simply checking the type on each iteration might produce inconsistent results.)

The check ((isMin && a < b) || (!isMin && a > b)) can be simplified to (isMin ? a < b : a > b).

aResultToken.value_int64 = param2.value_int64 is sufficient for both SYM_INTEGER and SYM_FLOAT, due to the use of union. Various parts of the code already rely on this.

param2 = param1 may produce larger code than necessary, due to copying the whole struct vs just the value and symbol. This may be a moot point if the code is changed to have a separate path for integer comparison, since in that case the temporary variables may be strongly-typed (__int64 or double).

@Ragnar-F
Copy link
Contributor Author

Thanks for the suggestions. They helped me to improve my understanding of the code.

To avoid inconsistent results due different number types, I've reworked the code. Integers and floats are now compared separately to determine the lowest/highest value respectively. Only then, the found integer and the found float are compared with each other.

@Lexikos Lexikos merged commit ef4be3a into AutoHotkey:master Dec 10, 2017
Lexikos added a commit that referenced this pull request Dec 10, 2017
@Ragnar-F Ragnar-F deleted the minmax branch December 10, 2017 13:54
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.

7 participants