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

no while checks if scanning a capture #88

Merged
merged 5 commits into from
Jul 12, 2019

Conversation

msftrncs
Copy link
Contributor

@msftrncs msftrncs commented Feb 8, 2019

This is a possible solution to #87. This prevents _tokenizeString from performing a while rule check if the starting line position is not 0 upon entry to the function.

Maybe it would be better if _tokenizeString was not responsible for performing the while rules check, and it was instead performed by _tokenize, since _tokenize is always starting a fresh line, but that seemed like a significant change.

@msftrncs
Copy link
Contributor Author

msftrncs commented Feb 8, 2019

Note: with this solution, the while check still occurs in a capture if the capture is at the start of the line, in some wild cases that could still cause a problem.

I cannot really figure out what is going on in the failure of the CI. The test script is failing during the markdown part of the tests, but from what I can see of the markdown tests, there is nowhere that this change would have affected. Maybe I am not looking at the right tests?

@msftrncs
Copy link
Contributor Author

msftrncs commented May 2, 2019

@alexandrudima, any input on this? I did an experiment to find another solution:

	const noRulesOnStack = stack.getRule(grammar) instanceof IncludeOnlyRule;

and then use !noRulesOnStack instead of linePos == 0 to perform the while rules check. Also seems to work, prevents the while rules from running if the capture is at the beginning of a line, but it is probably more expensive.

@alexdima alexdima merged commit c349c70 into microsoft:master Jul 12, 2019
@alexdima
Copy link
Member

Thank you!

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

2 participants