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

ceil function fails to parse correctly #10

Closed
markhats opened this issue Sep 7, 2018 · 6 comments
Closed

ceil function fails to parse correctly #10

markhats opened this issue Sep 7, 2018 · 6 comments
Labels
bug Indicates an unexpected problem or unintended behavior

Comments

@markhats
Copy link
Contributor

markhats commented Sep 7, 2018

The ceil function is failing to parse properly. This is because when the parser gets to the 'e' in ceil it recognises it as TokenType.EFUNC. This then messes up the subsequent parsing. This issue will occur with all functions containing the 'e' character.

I guess it perhaps needs to check varBuffer or something before deciding to create the EFUNC token.

Code in question is around:

if (keywords.containsKey(si)) {

@fkleon
Copy link
Owner

fkleon commented Sep 23, 2018

Thanks for the bug report, I appreciate it!

The parser is long due for a rewrite, but I currently only have very limited time to work on this project.
I should get around at some point to fix this specific issue with the existing parser though. Feel free to submit a PR if you have some spare time on hand.

@tanish2k09
Copy link

It's been a while. I'm using math_expressions for a certain calculator I'm working on and I read this TODO note going through the code. This seems like it would make a nice addition and I'm interested in implementing it but I'd need some time to understand the structure of the library.

@fkleon
Copy link
Owner

fkleon commented Mar 8, 2020

Hi @tanish2k09 - thanks for offering to take this on.

If you're still interested I suggest you have a look at my previous comment here which outlines the parser implementation: #1 (comment)

I'm not averse to a completely rewrite of the lexer/parser using a suitable library instead of the home-grown current solution. But I'd be equally happy with a small fix to the existing code base for this particular issue.

@tanish2k09
Copy link

Hi @tanish2k09 - thanks for offering to take this on.

If you're still interested I suggest you have a look at my previous comment here which outlines the parser implementation: #1 (comment)

I'm not averse to a completely rewrite of the lexer/parser using a suitable library instead of the home-grown current solution. But I'd be equally happy with a small fix to the existing code base for this particular issue.

I think the best course of action would be to try to “port” over a reliable and well-established lib from one of the other languages like (Swift? C++?)

@tanish2k09
Copy link

Unless of course an edit is preferred over a rewrite

@fkleon
Copy link
Owner

fkleon commented Mar 8, 2020

Porting a specific library is one option, another one would be to switch to a grammar-based parsing approach to build the internal expression structure, e.g. via dart-petitparser.

I don't have a strong preference either way as to rewrite vs. fix. The advantage of the current shunting yard algorithm is that it's a classic and well understood algorithm, which is easy to implement. In math_expressions, it's just not very well implemented around handling ambiguity correctly, which can probably be fixed relatively easily, e.g. by prioritising longer tokens when trying to match function names.

That said, I've been wanting to explore grammar-based parsing, but haven't really gotten around to doing the research - merely due to a lack of time. There seems to be plenty of literature available that could be used as a reference, for example: A Step-by-Step Solution Methodology for Mathematical Expressions.

I'd say pick whatever approach interests you most given the time you want to spend on this.

I've added a failing test case in 2241566 a while ago, which might be useful for a fix.

@fkleon fkleon added the bug Indicates an unexpected problem or unintended behavior label Jul 24, 2020
@fkleon fkleon closed this as completed Sep 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Indicates an unexpected problem or unintended behavior
Projects
None yet
Development

No branches or pull requests

3 participants