Closed Bug 1333844 Opened 3 years ago Closed 3 years ago

Create helper method to call ICU string functions

Categories

(Core :: JavaScript: Internationalization API, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: anba, Assigned: anba)

Details

Attachments

(1 file, 1 obsolete file)

To avoid repeating the same char-buffer handling everywhere.
Attached patch bug1333844.patch (obsolete) — Splinter Review
Do you think lambdas are a good choice for this task?
Assignee: nobody → andrebargull
Attachment #8830419 - Flags: feedback?(jwalden+bmo)
Comment on attachment 8830419 [details] [diff] [review]
bug1333844.patch

Review of attachment 8830419 [details] [diff] [review]:
-----------------------------------------------------------------

This feels a little harder to read to me, in that there's magic goo between, say, PartitionNumberFormat and the unum_formatDoubleForFields that it really wants to perform.  Reading each example, I have trouble remembering the exact sense of the parameters, when I'm not seeing in context how/when the lambda-body will be evaluated.

At the same time, the buffer-sizing logic is ubiquitous enough, and easy enough to miscopy or incorrectly modify, that probably we don't have much choice about doing something like this.  And I don't really see an alternative to lambdas that's any better.

So at the end of the day, this looks okay.  Just one criticism: having watched shu put lambdas to good use in the frontend, I think I now prefer that lambdas never have capture-defaults, but rather they should explicitly capture exactly the enclosing identifiers they use.  Please convert all the |[&]| to |&foo| when |foo| is some large object or an object with constructor/destructor or an object whose value changes, or plain |foo| when the object is simply copied and not modified during the lambda's lifetime.
Attachment #8830419 - Flags: feedback?(jwalden+bmo) → feedback+
Attached patch bug1333844.patchSplinter Review
I've updated the patch to use explicit capture lists, but then multiple call sites exceeded the 99 chars maximum and wrapping the calls into multiple lines didn't look that clean from a aesthetic POV. To avoid this formatting issue I renamed |CallICUFunction| to just |Call|. Apart from that change, the patch is identical to the previous one.
Attachment #8830419 - Attachment is obsolete: true
Attachment #8837791 - Flags: review?(jwalden+bmo)
Attachment #8837791 - Flags: review?(jwalden+bmo) → review+
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/583e4b713f18
Create helper method to call ICU string conversion methods. r=Waldo
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/583e4b713f18
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.