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

localeCompare() doesn't return the expected value #13788

Closed
apptimise opened this issue Oct 16, 2016 · 7 comments
Closed

localeCompare() doesn't return the expected value #13788

apptimise opened this issue Oct 16, 2016 · 7 comments
Assignees

Comments

@apptimise
Copy link

@apptimise apptimise commented Oct 16, 2016

Minimal example:

var b = "aZ";
var a= "ab"
var ret = b.localeCompare(a); // returns -8 on Samsung Internet for Android 4.0

@emilio
Copy link
Member

@emilio emilio commented Oct 16, 2016

Hi, thanks for reporting this :)

@emilio
Copy link
Member

@emilio emilio commented Oct 16, 2016

We never call JS_SetLocaleCallbacks, so they should use the default. I'll try with a debug-mozjs build.

@Ms2ger
Copy link
Contributor

@Ms2ger Ms2ger commented Oct 18, 2016

Assigned to @emilio for investigation

@emilio
Copy link
Member

@emilio emilio commented Oct 18, 2016

This is not a bug, SpiderMonkey by default (in js::str_localecompare returns the difference between the first two different characters, which in this case is -8.

From the ECMA spec this is completely valid:

The actual return values are implementation-defined to permit implementers to encode additional information in the value, but the function is required to define a total ordering on all Strings. This function must treat Strings that are canonically equivalent according to the Unicode standard as identical and must return 0 when comparing Strings that are considered canonically equivalent.

So I'm closing this, feel free to reopen if anyone disagrees with this diagnostic :)

@emilio emilio closed this Oct 18, 2016
@apptimise
Copy link
Author

@apptimise apptimise commented Oct 18, 2016

I don't agree. The result of the comparison should be in the range {-1, 0, +1} http://www.w3schools.com/jsref/jsref_localecompare.asp
Even if the return value is "implementation-defined", it should return a positive number. "aZ" is definitely larger than "ab". All other browsers return "+1". It should be okay to return "+8" or a positive number, but not "-8"! It changes the whole functionality

@emilio
Copy link
Member

@emilio emilio commented Oct 18, 2016

Hmm... Ok, so this is spec compliant (because the spec only requires to define a total order), and the current behavior is the same of strcoll, which is sort of the expected thing.

That being said, since nobody else does this, I guess it makes sense to switch.

Seems like the easiest way to get this working is compiling SM with EXPOSE_INTL_API, so we get the self-hosted localeCompare, which ends-up calling http://searchfox.org/mozilla-central/source/js/src/builtin/Intl.cpp#1083.

@jdm, @Ms2ger, do you think that's reasonable? I can try to do that tomorrow, I guess.

@emilio emilio reopened this Oct 18, 2016
@jdm
Copy link
Member

@jdm jdm commented Oct 18, 2016

Seems reasonable.

emilio added a commit to emilio/mozjs that referenced this issue Oct 19, 2016
See servo/servo#13788.

I've tried a Servo build with this and have seen no problem, and now stuff like
localeCompare do the right thing.
bors-servo added a commit to servo/mozjs that referenced this issue Oct 19, 2016
Build with intl API.

See servo/servo#13788.

I've tried a Servo build with this and have seen no problem, and now stuff like
localeCompare do the right thing.

r? @jdm

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/mozjs/107)
<!-- Reviewable:end -->
emilio added a commit to emilio/servo that referenced this issue Oct 19, 2016
Fixes servo#13788
emilio added a commit to emilio/servo that referenced this issue Oct 19, 2016
Fixes servo#13788
bors-servo added a commit that referenced this issue Oct 19, 2016
Update mozjs_sys to expose proper locale callbacks.

<!-- Please describe your changes on the following line: -->

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #13788 (github issue number if applicable).

<!-- Either: -->
- [ ] There are tests for these changes OR

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

Fixes #13788

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13836)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this issue Oct 24, 2016
Update mozjs_sys to expose proper locale callbacks.

<!-- Please describe your changes on the following line: -->

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #13788 (github issue number if applicable).

<!-- Either: -->
- [ ] There are tests for these changes OR

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

Fixes #13788

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13836)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this issue Oct 24, 2016
Update mozjs_sys to expose proper locale callbacks.

<!-- Please describe your changes on the following line: -->

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #13788 (github issue number if applicable).

<!-- Either: -->
- [ ] There are tests for these changes OR

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

Fixes #13788

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13836)
<!-- Reviewable:end -->
emilio added a commit to emilio/servo that referenced this issue Oct 24, 2016
Fixes servo#13788
emilio added a commit to emilio/servo that referenced this issue Oct 24, 2016
Fixes servo#13788
bors-servo added a commit that referenced this issue Oct 24, 2016
Update mozjs_sys to expose proper locale callbacks.

<!-- Please describe your changes on the following line: -->

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #13788 (github issue number if applicable).

<!-- Either: -->
- [ ] There are tests for these changes OR

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

Fixes #13788

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13836)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this issue Oct 25, 2016
Update mozjs_sys to expose proper locale callbacks.

<!-- Please describe your changes on the following line: -->

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #13788 (github issue number if applicable).

<!-- Either: -->
- [ ] There are tests for these changes OR

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

Fixes #13788

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13836)
<!-- Reviewable:end -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

4 participants
You can’t perform that action at this time.