Closed Bug 1319740 Opened 8 years ago Closed 8 years ago

Make getDisplayNames' MatchPart handle \0 in the pattern string


(Core :: JavaScript: Internationalization API, defect)

Not set



Tracking Status
firefox53 --- fixed


(Reporter: zbraniecki, Assigned: Waldo)




(4 files, 1 obsolete file)

This is a followup to bug 128677 comment 24. Jeff wrote:

Oh, ugh.  I just realized, this quite possibly/probably will result (since we're interpeting these strings as null-terminated) that this probably messes up handling of "dates/fields/year\0EXTRA".  Sigh.  Run with this for now, but file a followup (assign to me) to figure out how to address this.  I have an idea for fixing this, but it's probably not worth me talking you through executing it.
Depends on: 1287677
A couple mini-cleanupish patches before the ultimate fix.
Attachment #8816352 - Flags: review?(arai.unmht)
Assignee: nobody → jwalden+bmo
Attachment #8816353 - Flags: review?(arai.unmht)
This makes ComputeDisplayNames a bit more readable overall.  This could be a lambda, now -- but if I did that, the next patch (which templatizes this stuff based on string character type) would be a no-go without C++14's template lambdas that we can't quite use yet.

I could say it later, or now, but there's an argument to have about whether templating this parsing, or just copying to a two-byte chars array and parsing only that version, is preferable.  Efficiency doesn't matter much.  Mostly I do this because forked handling is generally pretty common and it seems best not to deviate.  I suspect un-templating and dealing only in two-byte things could be done if desired -- but probably best to say so wrt *this* patch, because moving this code as few times as desirable seems preferable.
Attachment #8816355 - Flags: review?(arai.unmht)
Missing one mini-fix.
Attachment #8816360 - Flags: review?(arai.unmht)
Attachment #8816355 - Attachment is obsolete: true
Attachment #8816355 - Flags: review?(arai.unmht)
Working with two different iterators is distinctly less pleasant than working with one plus null-termination.  Not sure anything can be done about it -- prescanning for nulls seems not worth it enough to do, right?
Attachment #8816362 - Flags: review?(arai.unmht)
Attachment #8816352 - Flags: review?(arai.unmht) → review+
Comment on attachment 8816353 [details] [diff] [review]
Rearrange a few lines

Review of attachment 8816353 [details] [diff] [review]:

::: js/src/builtin/Intl.cpp
@@ +3047,2 @@
>      // 1. Assert: locale is a string.
>      MOZ_ASSERT(args[0].isString());

this assertion won't be necessary if we're doing toString in next line.

@@ +3054,2 @@
>      // 2. Assert: style is a string.
>      MOZ_ASSERT(args[1].isString());

here too.

@@ +3061,2 @@
>      // 3. Assert: keys is an Array.
>      MOZ_ASSERT(args[2].isObject());

and here.
Attachment #8816353 - Flags: review?(arai.unmht) → review+
See Also: → 1321771
Attachment #8816360 - Flags: review?(arai.unmht) → review+
Comment on attachment 8816362 [details] [diff] [review]
Parametrize ComputeSingleDisplayName based on the character type of the key string, and iterate through the string using iterators, not using null-termination

Review of attachment 8816362 [details] [diff] [review]:

::: js/src/builtin/Intl.cpp
@@ +3330,5 @@
>      for (uint32_t i = 0; i < keys->length(); i++) {
>          if (!GetElement(cx, keys, keys, i, &v))
>              return false;
>          pattern.clear();

this and the definition of pattern should be removed now.
Attachment #8816362 - Flags: review?(arai.unmht) → review+
Pushed by
Add a slash-matching function rather than using MatchPart.  r=arai
Rearrange a few lines.  r=arai
Move display-name computation into its own function.  r=arai
Parametrize ComputeSingleDisplayName based on the character type of the key string, and iterate through the string using iterators, not using null-termination.  r=arai
You need to log in before you can comment on or make changes to this bug.