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

Fixes bug in string parsing when \\ immediately precedes \" #18

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

sysread
Copy link

@sysread sysread commented Dec 21, 2019

This addresses the bug in issue 17 by modifying the string regex to handle an escaped character immediately preceded by an escaped backslash.

@karupanerura
Copy link
Owner

Thank you for reporting and the patch.

TOML::Parser has strict_mode for compatibility, but it's falsy by default.
Your implementation is correct in the spec, but it breaks compatibility.

SEE: https://metacpan.org/pod/TOML::Parser#my-$parser-=-TOML::Parser-%3Enew(\%args)

And the same problems exist on some code points. so I want to fix these problems by one solution (using the same approach for all same problems).
But, your solution needs a very large regexp. If this solution is used for all problems, it not good for code readability.

So I want to consider another solution to the issue.
Do you have any ideas?

@sysread
Copy link
Author

sysread commented Jan 13, 2020

I'm not certain I understand your comments about strict mode and compatibility. Are you referring to the character class for matching escapes and code points? If you wish it to be less strict, that character class could probably be swapped for something less picky.

As far as the size of the regex, I could remove the use of /x, which I used to make it more legible while working out how to fix the problem. Is that what you meant?

Regarding the travis-ci failures, I think you need to add the travis-ci helper for perl:

- eval $(curl https://travis-perl.github.io/init) --auto --always-upgrade-modules

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