adletec / sonic

sonic is a rapid evaluation engine for mathematical expressions. It can parse and execute strings containing mathematical expressions.
MIT License
17 stars 4 forks source link

Arguments preceded by a unary minus don't count towards argument count. #37

Closed johnnyontheweb closed 1 year ago

johnnyontheweb commented 1 year ago

Hello all, merging the latest commits, I noticed some errors on calculation of the following expressions. I also have some questions:

        private static bool IsStartOfArgument(Stack<ValidationContext> contextStack, Token previousToken)
        {
            return IsFunctionOnTopOfStack(contextStack) && (
                previousToken.TokenType == TokenType.ArgumentSeparator ||
                previousToken.TokenType == TokenType.LeftParenthesis || IsUnaryOperation(previousToken)
            );
        }

Finally, I have a lot of script to be corrected. By now I decided to not update the library (prior to October 3rd), but in the future I want to staty up to date. Can you help on this? thanks in advance

vlow commented 1 year ago

Hi @johnnyontheweb,

let me take on your questions inline:

Must I use both ".UseCulture(Globalization.CultureInfo.InvariantCulture)" and ".UseArgumentSeparator(","c)" ?

No, you don't: the Evaluator will always default to , as argument separator for cultures with . as decimal separator and to ; for cultures with , as decimal separator. Since InvariantCulture uses . as decimal separator, , will be the argument separator by default.

Using .UseArgumentSeparator is only necessary for cases in which the default doesn't work for you and you want to override it.

"myfunc(var==0,-1.0)" which works as myfunc(condition,double) The unary operation causes the second argument to be not counted, hence the error "1 args instead of 2". I tried to correct the function IsStartofArgument by adding IsUnaryOperation, like this:

You're right about this being a bug, but your fix won't work: the problem is actually that the unary minus itself should be seen as the start of the argument. In other words 1.0 isn't the start of the argument, - is. The method IsStartOfArgument is working as designed, but it's actually not being called in case of a unary minus.

I just pushed a test and a fix to a bugfix branch which takes care of that (168ae5e).

Apart from that, from the engine's perspective, there is no condition token type. Boolean operations result in double values, so "var==0" will evaluate to 1.0 (true) or 0.0 (false) respectively.

"and(T==1,N>-(Af))"

You don't actually need to define a function like that, you could use the && operator. But if you prefer a custom function, I assume you're right about this being a consequence of your changes to the code.

I'm pushing the fix to a quick bugfix release. Keep me posted if it solves your problem :)

vlow commented 1 year ago

Closing this for now, since the fix just got released. Please re-open, if you're still encountering InvalidArgumentCountParseExceptions when using arguments preceded by a unary minus.

johnnyontheweb commented 1 year ago

Thanks a lot, it seems everything's fine now! ps. I know there's no condition token type, I meant that the first argument is an expression like "var==0".

vlow commented 1 year ago

Great, thanks for reporting the bug :)