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 parser not allowing space chars in arguments. #1

Merged
merged 2 commits into from
Oct 15, 2013

Conversation

catageek
Copy link
Contributor

This PR fixes the instruction parser that was splitting the all lines using commas and space.

The parser now extract the instruction using space separator, and separate arguments using comma separator.

This allows to have space in arguments, like in the string "[index + 1]" which was previously refused.

@ijc8
Copy link
Owner

ijc8 commented Oct 13, 2013

Looks good, thanks for the contribution! I notice, though, (from testing with snake) that this can lead to arguments being passed to handleArgument with trailing spaces, which causes a few things (label lookup, handleStack, and the closing bracket check) to fail; calling String.trim on the arguments at any point before they reach handleArgument fixes this.

@catageek
Copy link
Contributor Author

Your regex was implicitly removing the trailing spaces. I added a trim() call where I think it is the most appropriate, after the comment part removal and before other operations.

Your link to snake does not work. I find this one that does not load for another reason : label references in DAT lines are not supported.

ijc8 added a commit that referenced this pull request Oct 15, 2013
Fix parser not allowing space chars in arguments.
@ijc8 ijc8 merged commit 852722d into ijc8:master Oct 15, 2013
@ijc8
Copy link
Owner

ijc8 commented Oct 15, 2013

Alright, great. As for the broken link (I think the site just went down), here's a mirror: http://pastebin.com/yCJ1ubCx

@catageek catageek deleted the FixInstructionParser branch October 16, 2013 19:56
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.

2 participants