-
Notifications
You must be signed in to change notification settings - Fork 47
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
Update nex.go #53
base: master
Are you sure you want to change the base?
Update nex.go #53
Conversation
The formatting fixes look okay, but these should be in a separate commit please. |
File: fxtest |
} | ||
if stopped { | ||
break | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think you actually need to rip out the ability to stop. The problem is the for
loop and default
case in the select-- it causes the select to instantly complete, and the for loop to spin in a busy wait.
All you really need is something like this:
select {
case <- ch_stop:
return
case ch <- frame{matchi, text, line, column}:
}
We don't need the inner for
loop, or the sent / stopped vars.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds sensible. But what I would comment is that what ever happens seems to build up, what I found was that on a successful compile the channel were left open, but closing them did not help. Calling Stop() did close them but not solve the problem. Despite apparently always managing to escape the select, my run of a few hundred unit tests would start off slow and get slower and slower until something in golang exploded.
I will give your suggestion a try when I have the time and see what happens, but simplification is always good and the stop channel seems unneeded.
I really think the scanner and go subprocess is also over complex. Back in the day lexers used to work without a subprocess and managed to support "unput"!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
well yeah, that's why #38 ... but it's complicated to move all the state into struct fields that is currently being closed over
No description provided.