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 stringlib against non-ascii chars #184

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

masaedw
Copy link

@masaedw masaedw commented Feb 15, 2017

Some string functions ignore upper byte of chars of strings.
This causes unexpected behaviors.

ex)

  • "書籍" (U+66F8, U+7C4D) matches "糸筍" (U+7CF8, U+7B4D)
  • "[Aそ]" matches "㍝"

See the test cases.

I confirmed that all test in MoonSharp.Interpreter.Tests.net35-client passed using moonsharp_dev_minimal.sln.

@masaedw masaedw changed the title Fix stringlib Fix stringlib against non-ascii chars Feb 17, 2017
wledfor2 added a commit to wledfor2/moonsharp that referenced this pull request Oct 8, 2017
Eir-nya added a commit to RhenaudTheLukark/CreateYourFrisk that referenced this pull request Sep 4, 2019
Merged the lovely changes from moonsharp-devs/moonsharp#184 into CYF

* Fixed a bug where some `string` library functions would incorrectly match characters that only have the same lower byte as the ones you're trying to match.

Fix by masaedw on GitHub
Copy link
Contributor

@LimpingNinja LimpingNinja left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added extra eyes (@Benjamin-Dobell) for review, I ran a few tests and didn't see anything out of the ordinary. Always a bit concerned when removing the byte casting here as it may be the easiest way to fix it, but uncertain (since encoding is not my forte) if this is the right way to fix this.

Copy link
Contributor

@LimpingNinja LimpingNinja left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ensure code changes on PR cover the other instances of this while we still have them

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