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

Fix allowing arithmetic expressions without spaces and no need for extra operator e.g. "4+-5". "4-5" works. #4

Merged
merged 1 commit into from
Apr 5, 2023

Conversation

harrybits
Copy link

Hello @arothuis! I realize this project is a small demonstration of ANTLR4, but I was interested in expanding the calculator's function and became annoyed with the strict formatting of the input. For example, I wanted to enter "6-4" instead of "6 + -4". No spaces to worry about. This PR accomplishes that objective. The trade-off is a slight muddling of your clear demo code.

This can be considered a hack, but it didn't seem too egregious and the benefit is nice. Also, a discussion of the fix could be beneficial to new users of ANTLR. Let me know what you think.

There are a few more minor edits to address compilation warnings.

@arothuis
Copy link
Owner

arothuis commented Apr 5, 2023

Hi there!

First of all, thanks for your work, your tests and your clear explanation. I see that the same issue has been reported by someone else here (#3), but I have only recently re-enabled notifications.

I'm not a fan of adding an expression to fix whitespace issues.
The problem we're seeing here is not about incorrectly handling spaces, it is rather about how I have chosen to define tokens and expressions in the grammar.

A more elegant approach could be as follows:

  • define the NUMBER token to be positive only in the grammar
  • add an expression for negation, i.e. '-' right=expression # Negation
  • implement correct behaviors in CalculationListener::exitNegation (e.g. - popped) and CalculationVisitor::visitNegation (e.g. - whatever is on the right)

This approach would also allow -(1 + 2) to work.

Would you like to try this approach?
If not, I'm down to fix this whenever I have some time this week.
I'll create an issue for some other fixes that would make this a better example (#5).


Context about this project:
I created this project to learn working with ANTLR so that I could point my students in the right direction. I have never worked with ANTLR and language grammars before, so mistakes were to be expected. Sadly, the assignment to work with ANTLR has been dropped from the curriculum, so I had no reason to update this example. I have contemplated archiving this repository, but I'd rather fix it so we have a decent example for people to study.

@harrybits
Copy link
Author

I prefer the negation strategy. It's cleaner and more logical. I'll spend some more time on this for posterity.

…ween operators and allows for negation unary expressions.
@harrybits
Copy link
Author

Implemented @arothuis 's strategy outlined in his comment above. Dropped my last commit containing the previous fix.

@arothuis-hu
Copy link
Collaborator

Nice! Thanks for all the work.
Merging!

@arothuis-hu arothuis-hu merged commit 208daa1 into arothuis:master Apr 5, 2023
@arothuis arothuis mentioned this pull request Apr 5, 2023
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

3 participants