ICU-22781 Support Arbitrary Constant Unit Formatting #3381

younies · 2025-02-10T14:49:13Z

Description:

Added support for constant denominators in MeasureUnit and LongNameHandler
Updated MeasureUnit serialization and product methods to handle constant denominators
Added test cases for formatting units with arbitrary constant denominators
Added comprehensive test coverage for complex unit formatting scenarios

Related ticket that needs to be addressed: https://unicode-org.atlassian.net/browse/ICU-23039

Checklist

Required: Issue filed: ICU-22781
Required: The PR title must be prefixed with a JIRA Issue number. Example: "ICU-1234 Fix xyz"
Required: Each commit message must be prefixed with a JIRA Issue number. Example: "ICU-1234 Fix xyz"
Issue accepted (done by Technical Committee after discussion)
Tests included, if applicable
API docs and/or User Guide docs changed or added, if applicable

See unicode-org#3381

jira-pull-request-webhook · 2025-02-10T14:49:36Z

Hooray! The files in the branch are the same across the force-push. 😃

~ Your Friendly Jira-GitHub PR Checker Bot

See unicode-org#3381

jira-pull-request-webhook · 2025-02-10T14:49:49Z

Hooray! The files in the branch are the same across the force-push. 😃

~ Your Friendly Jira-GitHub PR Checker Bot

See unicode-org#3381

jira-pull-request-webhook · 2025-02-10T15:21:48Z

Hooray! The files in the branch are the same across the force-push. 😃

~ Your Friendly Jira-GitHub PR Checker Bot

richgillam

I had a few questions...

richgillam · 2025-02-10T21:51:48Z

icu4c/source/i18n/measunit_extra.cpp

-    if (impl.singleUnits.length() > 1) {
+
+    if (this->getConstantDenominator(status) != 0 && other.getConstantDenominator(status) != 0) {
+        // Cannot multiply units that both of them have a constant denominator


There is only one constant denominator in a compound unit.
Therefore, we Cannot multiply units that both of them have a constant denominator

I have updated the comment

Sure, but if you multiply together two units that both have constant denominators, wouldn't the constant denominator of the result just be the product of the original two units' constant denominators? That is, if you multiply liter-per-100-kilometer by liter-per-100-kilometer, wouldn't the result be square-liter-per-10000-square-kilometer?

You are right, but I am looking at it from another perspective.

liter-per-100-kilometer multiplied by liter-per-100-kilometer will result in square-liter-per-100-100-square-kilometer, which leads to duplication in unit constants.

but we can also, do it the way you have suggested, but because I need to check that the multiplication should not exceed LONGMAX, let's add a TODO and try to fix it later. (we can quickly discuss it too in the ICU meeting)

It'd be interesting to hear what Mark thinks. But I'm fine with what you're suggesting here.

richgillam · 2025-02-10T21:53:55Z

icu4c/source/i18n/measunit_extra.cpp

+    }
+
+    impl.constantDenominator =
+        this->getConstantDenominator(status) + other.getConstantDenominator(status);


You can add here because you know one or the other of these is going to be 0? If that's what you're doing, I'd recommend documenting it.

richgillam · 2025-02-10T22:03:10Z

icu4c/source/test/intltest/numbertest_api.cpp

+         u"3 meters per 100-second-kelvin"},
+        {"meter-per-1000", 1, UNUM_UNIT_WIDTH_FULL_NAME, Locale::getEnglish(), u"1 meter per 1000"},
+        {"meter-per-1000-second", 1, UNUM_UNIT_WIDTH_FULL_NAME, Locale::getEnglish(),
+         u"1 meter per 1000-second"},


Didn't you have a bunch of logic for handling plurals in the code above? Shouldn't this be "1 meter per 1000 seconds" or something like that? And why is there a hyphen in "1000-second"? We wouldn't have that in English.

unfortunately, for now, it is just a plain number.

There is an issue for that: https://unicode-org.atlassian.net/browse/ICU-23039

It looks really weird, but since you have an outstanding ticket to improve this, I'll accept it for now.

richgillam · 2025-02-10T22:04:17Z

icu4c/source/test/intltest/numbertest_api.cpp

+         u"1 meter per 1-kilogram-second-kelvin"},
+        {"meter-second-per-kilogram-kelvin", 1, UNUM_UNIT_WIDTH_FULL_NAME, Locale::getEnglish(),
+         u"1 meter-second per kilogram-kelvin"},
+        {"meter-second-per-1000-kilogram-kelvin", 1, UNUM_UNIT_WIDTH_FULL_NAME, Locale::getEnglish(),


Same issue here (among other places): Shouldn't this be "meter-second per 1000 kilogram-kelvins"?

unfortunately, for now, it is just a plain number.

There is an issue for that: https://unicode-org.atlassian.net/browse/ICU-23039

richgillam · 2025-02-10T22:05:28Z

icu4c/source/test/intltest/numbertest_api.cpp

+         u"1 m⋅sec/1000⋅kg⋅K"},
+        {"meter-second-per-1000-kilogram-kelvin", 1, UNUM_UNIT_WIDTH_FULL_NAME, Locale::getGerman(),
+         u"1 Meter⋅Sekunde pro 1000⋅Kilogramm⋅Kelvin"},
+        {"meter-second-per-1000-kilogram-kelvin", 1, UNUM_UNIT_WIDTH_SHORT, Locale::getGerman(),


Mark had been arguing for special formatting for larger denominators, so that if you had "meter-per-1e6-kilogram", you'd get "meter per 1,000,000 kilograms". Did you do that? If so, you should have a test for it.

unfortunately, for now, it is just a plain number.

There is an issue for that: https://unicode-org.atlassian.net/browse/ICU-23039

richgillam · 2025-02-10T22:32:27Z

icu4c/source/test/intltest/numbertest_api.cpp

+    UnicodeString result =
+        formatter.locale("en-US").formatDouble(value, status).getOutputUnit(status).getIdentifier();
+    assertEquals("Testing default -u-mu- for en-US", MeasureUnit::getFahrenheit().getIdentifier(),
+                 result);


This is one of a number of cases where the code seems to have been reformatted with no substantive change. Those kinds of things make the diff a lot harder to read. Is the reformatting necessary?

I agree with you it is really annoying when the IDE format some part that are not necessary.

Sorry that I did not revert them, I am reverting them now.

See unicode-org#3381

jira-pull-request-webhook · 2025-02-11T18:51:35Z

Hooray! The files in the branch are the same across the force-push. 😃

~ Your Friendly Jira-GitHub PR Checker Bot

richgillam

Looks good, I think. I still think you can do the "right thing" in product(), but I don't wasn't to hold this up over that.

richgillam · 2025-02-11T21:46:02Z

icu4c/source/i18n/number_longnames.cpp

+        // "meter per 100 seconds" should be evaluated for correct singular/plural
+        // usage: "second" or "seconds"?
+        // Similarly, "kilogram per 1000 meters" should be checked for "meter" or
+        // "meters"?


Thanks for the TODO here. And if it was there all along, I'm sorry I didn't notice it and put two and two together before I added my previous comments.

No worries, it was there from the first version , but no problem :)

See unicode-org#3381

jira-pull-request-webhook · 2025-02-11T22:54:44Z

Hooray! The files in the branch are the same across the force-push. 😃

~ Your Friendly Jira-GitHub PR Checker Bot

younies added a commit to younies/icu that referenced this pull request Feb 10, 2025

ICU-22781 Support Arbitrary Constant Unit Formatting (C++)

29da2c6

See unicode-org#3381

younies force-pushed the cpp-format-arbitrary-constants branch from 6b1855c to 29da2c6 Compare February 10, 2025 14:49

younies added a commit to younies/icu that referenced this pull request Feb 10, 2025

ICU-22781 Support Arbitrary Constant Unit Formatting (C++)

5ad9adb

See unicode-org#3381

younies force-pushed the cpp-format-arbitrary-constants branch from 29da2c6 to 5ad9adb Compare February 10, 2025 14:49

younies requested review from sffc and richgillam February 10, 2025 14:50

younies added a commit to younies/icu that referenced this pull request Feb 10, 2025

ICU-22781 Support Arbitrary Constant Unit Formatting (C++)

d132924

See unicode-org#3381

younies force-pushed the cpp-format-arbitrary-constants branch from 6b8ba6f to d132924 Compare February 10, 2025 15:21

richgillam reviewed Feb 10, 2025

View reviewed changes

younies requested a review from richgillam February 11, 2025 15:01

younies changed the title ~~ICU-22781 Support Arbitrary Constant Unit Formatting (C++)~~ ICU-22781 Support Arbitrary Constant Unit Formatting Feb 11, 2025

This was referenced Feb 11, 2025

ICU-22781 Format Units with Arbitrary Constants #3369

Closed

ICU-22781 Support Formatting Units Constants (Java) #3361

Closed

younies added a commit to younies/icu that referenced this pull request Feb 11, 2025

ICU-22781 Support Arbitrary Constant Unit Formatting

4e1af73

See unicode-org#3381

younies force-pushed the cpp-format-arbitrary-constants branch from 9f703cf to 4e1af73 Compare February 11, 2025 18:51

richgillam previously approved these changes Feb 11, 2025

View reviewed changes

younies dismissed richgillam’s stale review via 54e8881 February 11, 2025 22:42

ICU-22781 Support Arbitrary Constant Unit Formatting

6954aac

See unicode-org#3381

younies force-pushed the cpp-format-arbitrary-constants branch from e323dc8 to 6954aac Compare February 11, 2025 22:54

younies requested a review from richgillam February 11, 2025 22:54

richgillam approved these changes Feb 11, 2025

View reviewed changes

younies merged commit 35c9778 into unicode-org:main Feb 12, 2025
101 checks passed

younies deleted the cpp-format-arbitrary-constants branch February 12, 2025 00:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ICU-22781 Support Arbitrary Constant Unit Formatting #3381

ICU-22781 Support Arbitrary Constant Unit Formatting #3381

younies commented Feb 10, 2025

jira-pull-request-webhook bot commented Feb 10, 2025

jira-pull-request-webhook bot commented Feb 10, 2025

jira-pull-request-webhook bot commented Feb 10, 2025

richgillam left a comment

richgillam Feb 10, 2025

younies Feb 11, 2025

richgillam Feb 11, 2025

younies Feb 11, 2025

richgillam Feb 11, 2025

richgillam Feb 10, 2025

younies Feb 11, 2025

richgillam Feb 10, 2025

younies Feb 11, 2025

richgillam Feb 11, 2025

richgillam Feb 10, 2025

younies Feb 11, 2025

richgillam Feb 10, 2025

younies Feb 11, 2025

richgillam Feb 10, 2025

younies Feb 11, 2025

richgillam Feb 11, 2025

jira-pull-request-webhook bot commented Feb 11, 2025

richgillam left a comment

richgillam Feb 11, 2025

younies Feb 11, 2025

jira-pull-request-webhook bot commented Feb 11, 2025

ICU-22781 Support Arbitrary Constant Unit Formatting #3381

ICU-22781 Support Arbitrary Constant Unit Formatting #3381

Conversation

younies commented Feb 10, 2025

Description:

Checklist

jira-pull-request-webhook bot commented Feb 10, 2025

jira-pull-request-webhook bot commented Feb 10, 2025

jira-pull-request-webhook bot commented Feb 10, 2025

richgillam left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jira-pull-request-webhook bot commented Feb 11, 2025

richgillam left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jira-pull-request-webhook bot commented Feb 11, 2025