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

Mathops extension #144

Merged
merged 4 commits into from Jan 10, 2014
Merged

Mathops extension #144

merged 4 commits into from Jan 10, 2014

Conversation

shimaore
Copy link
Contributor

This changeset modifies the mathops module by adding a math_rpn function which provides access (in an easily extensible way) to many more operations than the default math_eval parser, especially functions (the need for exp was the reason I wrote this). The math_rpn parser produces its output steps the way the original math_eval parser does, so that if the parsing is ever moved to fixup-time, the computed steps can be used without regards for the parser.

(The math_eval parser could also be modified to support the new operations but I didn't try to do that since the RPN syntax achieved what I needed.)

Submitting for review.

Add a new `math_rpn` function, and capacity to add new operators when using the RPN parser.
New operators (RPN only): `exp`, `floor`
@liviuchircu
Copy link
Member

As a side note, the module can still be improved with a series of fixup optimizations which include pvar handling (token should include an un-evaluated pv_spec_p struct) in order to avoid expression parsing at runtime. I should put this on the TODO list :)

A couple of quick ideas related to the pull request:

  • Reverse Polish notation is HIDEOUS. Humans hate it and parsers love it. I have some doubts about the module exposing a function that takes non-intuitive inputs. Building code on top of existing code (i.e. enhancing math_eval with maintaining proper operator priorities), and hiding the internal logic would be a better approach.
  • it looks like parsing will fail with "math_rpn("2 1 1 +*", "$var(res)")" (maybe I'm wrong!)
  • please use tabs for code indenting! (you can configure your editor afterwards for 2-char-wide tabs)
  • try to be consistent with existing code. Not "for ( i.." but "for (i...". Not "if(..." but "if (...")

To better understand reason 1), I give you the following exercise:
Convert "1 + exp(10, 5) - 2 * 2" in RPN. How long did it take you? :)

@shimaore
Copy link
Contributor Author

Thank you for your comments.

Reverse Polish notation is hideous.

YMMV. :) As an HP (RPN) user and someone who has written a Forth interpreter in his formative years, I don't have issues moving between RPN and infix. I saw a RPN evaluator, I needed to add a function to it, it was faster to write a parser for RPN than try to extend and fix the existing parser (accounting for arity, LR(1) parsing, etc.). I understand not everyone will find math_rpn attractive or useful.

On a practical (rather than aesthetic) point, the extensions to the evaluator still make sense (but they are missing parsing in math_eval), and math_rpn is a good way to write unit tests (from an OpenSIPS script) for the individual functions and the evaluator itself.

math_rpn("2 1 1 +*", "$var(res)")

I made the choice (same as in Forth) of using blank-separated words. So you'd effectively have to write this math_rpn("2 1 1 + *", "$var(res)").

I'll revise for code formatting.

@ghost ghost assigned liviuchircu Dec 6, 2013
liviuchircu added a commit that referenced this pull request Jan 10, 2014
Mathops extension: new RPN expression parsing function
@liviuchircu liviuchircu merged commit 5685379 into OpenSIPS:master Jan 10, 2014
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.

None yet

2 participants