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

Add checkEOL() to every command #19

Open
RFO-BASIC opened this issue Nov 11, 2012 · 5 comments
Open

Add checkEOL() to every command #19

RFO-BASIC opened this issue Nov 11, 2012 · 5 comments

Comments

@RFO-BASIC
Copy link
Owner

Most commands do not check to insure that there is no extraneous (non comment) characters after syntax processing on a command is done. This lack of checking can make debugging some types of coding errors difficult.

If you see a command that does not have a checkEOL(), put it in.....and TEST that change.

jMarcS added a commit to jMarcS/Basic that referenced this issue Nov 14, 2012
Added a version of checkEOL that takes no arguments
Same as existing checkEOL(false)
Changed all checkEOL(false) calls to checkEOL()
@jMarcS
Copy link
Collaborator

jMarcS commented Nov 14, 2012

I've added a second checkEOL() that takes no arguments. It's exactly like checkEOL(false). In the same check-in, I changed every checkEOL(false) to checkEOL(). There is absolutely no other change in this check-in. That makes it pretty safe, but I tested it anyway -- well, almost. I tested 138 of the 139 occurrences. The one I did not check is in executeCLIENT_GETFILE. I'll get to BASIC! sockets soon, I promise.
The next step is going to be to changed every checkEOL(true) to checkEOL(false), but that will require some extra work almost everywhere it happens.

@jMarcS
Copy link
Collaborator

jMarcS commented Nov 26, 2012

Round 2: the intent was just to remove checkEOL(boolean), leaving only the non-parameterized checkEOL(). That meant adjusting the places that used to rely on checkEOL(true) bumping the line number, distinguishing correct from incorrect ones. And that was a good excuse to touch a lot of code: added return false to some places that call RunTimeError() and removed some of the redundant SyntaxError() calls. I also reordered some code in getVar() and doUserFunction(), for readability and maybe a little performance. With a very few exceptions I thoroughly tested everything I touched.

One last-minute trivial change that affected dozens and dozens of lines I did not test: everywhere '};" was functionally identical to "}" I took out the semi-colon. I let the compiler test those.

This is a big, messy change, and I don't think there's anything critical in there, so I plan to hold off pushing this to RFO-BASIC until after v01.71 is out, Besides, that'll give me time to deal with the merge.

@jMarcS
Copy link
Collaborator

jMarcS commented Nov 29, 2012

Round 2 is checked in.

jMarcS added a commit to jMarcS/Basic that referenced this issue Dec 1, 2012
@jMarcS
Copy link
Collaborator

jMarcS commented Dec 1, 2012

Issue #41 gave me a close look at executeIF and its ilk. There was one checkEOL() missing (extra characters after IF with no THEN). That case still got a syntax error; all that was missing was the standard runtime error message.

But I wrapped in another change, as the commit comment says. A while back we got a big performance improvement by changing a lot of Integer objects to int primitives. This go-round I changed constants pushed onto the IfElseStack from int to Integer. The idea was to save a lot of boxing and unboxing, making all the conditions just Object reference compares. It didn't actually help any -- the benchmark results before and after are indistinguishable. Oh well, live and learn.

It did have one side-effect: switch cases can't be Objects, so the switch at the top of StatementExecuter is now an if-else tree.

Other minor changes:

  • executeIf -- consolidated "return true" statements, eliminated SingleLine variable, readability tweaks
  • SingleLineIf -- reduced String creation, eliminated SaveELB variable.
  • executeELSE -- fixed a comment, combined parallel IfElseStack.push calls
  • executeELSEIF -- fixed a comment, consolidated "return true", made it look more like executeIf

Testing: breakpoints showed f04_if_else.bas covers all the positive paths. I just needed to inject manual syntax errors to confirm the one added checkEOL works and all other syntax errors are still reported.

@jMarcS
Copy link
Collaborator

jMarcS commented Jan 10, 2013

Covered most BT commands with fix for Issue 47 on 2012/12/12.
Covered most BYTE and TEXT file commands with fix for issue 51 on 2013/01/06.
Covered SQL commands with groundwork for issue 61 on 2013/01/27.
Covered SOCKET commands with groundwork for issues 1&62 on 2013/02/02.
Covered TIMER commands with groundwork for issue 72 on 2013/02/24.
Covered FN.DEF, MyPhoneNumber, SOUNDPOOL and ARRAY commands in commits in March, 2013.
Covered LIST commands 2013/04/01.

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

No branches or pull requests

2 participants