Redmine#2111: eval() function for math evaluation #882
Conversation
|
There are lots of warnings when compiling; they can be cleaned up or silenced if the general approach is OK. |
|
Any blockers to merging this? |
|
I like the idea! Some thoughts: I would disable the Makefile rule. If we don't build the tool by default, people will just be confused if Git happens to check out the files with slightly different timestamps and 'make' tries to rebuild it. Put a comment at the top of the peg file instead on how to rebuild it. I also think we should include the tool if it is not common to have it. Make a 3rdparty folder or something like that. Otherwise we risk that other site going down and us being stranded with no way to generate a new file. Remember documentation! |
| { | ||
| vars: | ||
| "values[0]" string => "x"; | ||
| "values[1]" string => "+ 200"; |
There was a problem hiding this comment.
It's not mathematically valid so I disallowed it. libmatheval agrees with me FWIW, but I can allow it if you think it's needed.
There was a problem hiding this comment.
To me it seems a bit odd, but I have no strong preference.
When I say include, I don't mean build. |
|
I'll write docs when the feature is merged. I'm OK with just including the compiled syntax, it's very unlikely to change, and removing the Makefile rule. We can put http://piumarta.com/software/peg/ under |
|
Yep, sounds good! |
|
Please don't merge, still under discussion. |
|
There is no license in the peg directory. We should make it clear it is MIT licensed. |
|
|
||
| #include <math.pc> | ||
|
|
||
| double EvaluateMathInfix(EvalContext *ctx, char *input, char *failure) |
There was a problem hiding this comment.
I would use const char * here, at least for input.
Is failure supposed to contain a message if the evaluation fails? How do we know peg will not write a longer message than we have allocated for?
There was a problem hiding this comment.
Input constified.
The failure is only populated once, with strcpy(yy->failure, "expression could not be parsed"). Should I use strncpy()?
|
OK, license brought in, inputs constified, and tests still pass. |
|
Alright, looks good with the fixes! Merge when you want! |
|
Can one of the admins verify this patch? |
Redmine#2111: eval() function for math evaluation
|
@vohi @kacfengine I will write the docs later today. |
Feature requested in https://cfengine.com/dev/issues/2111
This implementation uses a PEG (parsing expression grammar). The
pegtool used to compilemath.pegintomath.pc(a C file) is MIT licensed at http://piumarta.com/software/peg/ and is tiny, so it could be included in the distribution. I've includedmath.pcin a separate commit so you can test it, and added a rudimentaryMakefile.amrule to generate it viapeg -o math.pc math.peg.Note that unlike
lex, the expression parser here is reentrant and thread-safe. It works off a local context for every invocation, so there are no global variables. The trivial stack implementation resides in the local context as well.I am not sure about the mechanics of incorporating the
pegtool ormath.pcinto the Automake process properly.The list of functions and operators is trivial to extend through
math.pegand the grammar is (IMHO) much simpler to write and understand than the classic lex+yacc grammars, so I feel this is a good extensible solution.Acceptance test included. I will add documentation if this pull request is accepted in some form.