Use `Impl` libraries for all internal libraries #15

younies · 2020-07-04T01:15:39Z

This PR is the first PR among a series of PR to point all the internal code to use impl instances instead of the public ones.

Checklist

Issue filed: https://unicode-org.atlassian.net/browse/ICU-_____
Updated PR title and link in previous line to include Issue number
Issue accepted
Tests included
Documentation is changed or added

hugovdm

Just some initial things - not yet a thorough review.

The variouis functions aren't immediately clear to me, I'll probably write some notes as I go: if I have short proposed documentation sentences for each, do you suggest I just leave them as comments? Or better to simply push a commit to the branch?

icu4c/source/i18n/measunit_extra.cpp

hugovdm · 2020-07-06T16:43:55Z

icu4c/source/i18n/unitconverter.cpp

+        for (int32_t i = 0, baseUnitsCount = baseUnits.length(); i < baseUnitsCount; i++) {
+            baseUnits[i]->dimensionality *= singleUnit.dimensionality;
+            // TODO: Deal with SI-prefix
+            result.append(*baseUnits[i], status);


Can you add a unit test? Maybe with a commented-out test that would show what is currently broken (until you deal with SI prefixes)?

I will add a test cases that fails while I fixing this issue.

icu4c/source/i18n/unitconverter.cpp

icu4c/source/i18n/measunit_extra.cpp

icu4c/source/i18n/measunit_impl.h

icu4c/source/i18n/measunit_extra.cpp

hugovdm · 2020-07-10T23:08:55Z

I noticed a merge conflict due to my -p4- -> -pow4- fix. For convenience, I went ahead and merged units-staging and fixed this.

icu4c/source/i18n/complexunitsconverter.cpp

icu4c/source/i18n/measunit_extra.cpp

icu4c/source/i18n/unitconverter.cpp

icu4c/source/i18n/unitconverter.h

icu4c/source/test/intltest/unitstest.cpp

sffc

Thanks. Please also see the other open threads.

sffc

Almost done!

icu4c/source/i18n/complexunitsconverter.cpp

icu4c/source/i18n/measunit_extra.cpp

icu4c/source/i18n/unitconverter.h

icu4c/source/i18n/measunit_impl.h

icu4c/source/i18n/complexunitsconverter.cpp

icu4c/source/i18n/measunit_impl.h

icu4c/source/i18n/unitsrouter.cpp

icu4c/source/i18n/unitconverter.cpp

icu4c/source/i18n/measunit_impl.h

sffc · 2020-07-22T19:25:15Z

There's still an error:

Error: in group units: i18n/measunit.o imports icu_67::MeasureUnitImpl::copy(UErrorCode&) const but units does not depend on units_extra (for i18n/measunit_extra.o)

IIRC, this is why the copy function is defined in the header file. If you want to move the copy function to the cpp file, put it in measunit.cpp instead of measunit_extra.cpp.

Commit: cc786cf Use `Impl` libraries for all internal libraries

Use `Impl` libraries for all internal libraries

PR: icu-units#15 Commit: cc786cf

Add precision to the output of UnitsRouter#route PR: icu-units#10 Commit: 030bda3 Use `Impl` libraries for all internal libraries PR: icu-units#15 Commit: cc786cf Sort the units in ComplexUnitConverter PR: icu-units#6 Commit: f65b181

younies added 3 commits July 2, 2020 13:20

start converting to use impl internally

e023aef

Merge branch 'units-staging' of github.com:icu-units/icu into use-impl

110cd72

converting extractConvertibility to use MeasureUnitImpl

5942b4c

younies requested review from sffc and hugovdm July 4, 2020 01:15

hugovdm requested changes Jul 6, 2020

View reviewed changes

sffc requested changes Jul 6, 2020

View reviewed changes

icu4c/source/i18n/unitconverter.cpp Outdated Show resolved Hide resolved

icu4c/source/i18n/measunit_extra.cpp Outdated Show resolved Hide resolved

icu4c/source/i18n/measunit_impl.h Outdated Show resolved Hide resolved

icu4c/source/i18n/measunit_extra.cpp Outdated Show resolved Hide resolved

Merge branch 'units-staging' into use-impl

3b75ac9

sffc linked an issue Jul 14, 2020 that may be closed by this pull request

ICU-21536 Fix remaining TODOs and FIXMEs in units code #28

Open

sffc added this to the 2020-Q3-m1 milestone Jul 14, 2020

younies added 3 commits July 19, 2020 23:17

fully switch to use impl libraries internally

dd3c4b8

Merge branch 'use-impl' of github.com:icu-units/icu into use-impl

864b4a7

remove extractConvertibility that are depending on MeasureUnit

e402861

younies changed the title ~~Use MeasureUnitImpl for check convertibility~~ Use Impl libraries for all internal libraries Jul 20, 2020

fix typos and memory issues

98e3565

sffc requested changes Jul 20, 2020

View reviewed changes

fix memory leak

702347d

hugovdm mentioned this pull request Jul 20, 2020

Support complex units as mixed units #40

Closed

sffc requested changes Jul 20, 2020

View reviewed changes

add more functionality to MeasureUnitImpl and clean ComplexUnitConverter

9c28a87

younies requested review from hugovdm and sffc July 21, 2020 01:29

younies added 2 commits July 21, 2020 04:28

small fix in ConversionRate

c4cd5cf

Merge branch 'units-staging' of github.com:icu-units/icu into use-impl

c680dbe

sffc requested changes Jul 21, 2020

View reviewed changes

hugovdm requested changes Jul 21, 2020

View reviewed changes

icu4c/source/i18n/measunit_impl.h Outdated Show resolved Hide resolved

icu4c/source/i18n/complexunitsconverter.cpp Outdated Show resolved Hide resolved

hugovdm reviewed Jul 21, 2020

View reviewed changes

icu4c/source/i18n/measunit_impl.h Outdated Show resolved Hide resolved

hugovdm reviewed Jul 21, 2020

View reviewed changes

icu4c/source/i18n/unitsrouter.cpp Outdated Show resolved Hide resolved

hugovdm reviewed Jul 21, 2020

View reviewed changes

icu4c/source/i18n/unitconverter.cpp Outdated Show resolved Hide resolved

younies requested review from hugovdm and sffc July 22, 2020 13:21

younies self-assigned this Jul 22, 2020

sffc approved these changes Jul 22, 2020

View reviewed changes

hugovdm reviewed Jul 22, 2020

View reviewed changes

icu4c/source/i18n/measunit_impl.h Show resolved Hide resolved

younies added 2 commits July 22, 2020 20:14

fix appendAll

780f455

remove MeasureUnitImpl#appendAll

ed3dacc

hugovdm approved these changes Jul 22, 2020

View reviewed changes

move MeasureUnitImpl#copy to measunit.cpp

70c69cf

younies merged commit cc786cf into units-staging Jul 23, 2020

younies deleted the use-impl branch July 23, 2020 00:30

hugovdm pushed a commit to hugovdm/icu that referenced this pull request Jul 23, 2020

PR: icu-units#15

f808da1

Commit: cc786cf Use `Impl` libraries for all internal libraries

hugovdm pushed a commit to hugovdm/icu that referenced this pull request Jul 23, 2020

PR: icu-units#15

5d496ae

Commit: cc786cf Use `Impl` libraries for all internal libraries

hugovdm pushed a commit to hugovdm/icu that referenced this pull request Jul 23, 2020

PR: icu-units#15

0c66ef6

Commit: cc786cf Use `Impl` libraries for all internal libraries

hugovdm pushed a commit to hugovdm/icu that referenced this pull request Jul 23, 2020

PR: icu-units#15

6b68df4

Commit: cc786cf Use `Impl` libraries for all internal libraries

younies added a commit that referenced this pull request Jul 24, 2020

Merge pull request #15 from icu-units/use-impl

67fc641

Use `Impl` libraries for all internal libraries

hugovdm pushed a commit to hugovdm/icu that referenced this pull request Aug 3, 2020

Use Impl libraries for all internal libraries

ad87e9a

PR: icu-units#15 Commit: cc786cf

hugovdm pushed a commit to hugovdm/icu that referenced this pull request Aug 3, 2020

Use Impl libraries for all internal libraries

f02673b

PR: icu-units#15 Commit: cc786cf

hugovdm pushed a commit to hugovdm/icu that referenced this pull request Aug 15, 2020

Use Impl libraries for all internal libraries

fed9df6

PR: icu-units#15 Commit: cc786cf

hugovdm pushed a commit to hugovdm/icu that referenced this pull request Aug 28, 2020

Use Impl libraries for all internal libraries

024792a

PR: icu-units#15 Commit: cc786cf

hugovdm pushed a commit to hugovdm/icu that referenced this pull request Sep 1, 2020

ICU-20568 Use Impl libraries for all internal libraries

e847337

PR: icu-units#15 Commit: cc786cf

hugovdm pushed a commit to hugovdm/icu that referenced this pull request Sep 2, 2020

ICU-20568 Use Impl libraries for all internal libraries

469ace9

PR: icu-units#15 Commit: cc786cf

hugovdm pushed a commit to hugovdm/icu that referenced this pull request Sep 2, 2020

ICU-20568 Use Impl libraries for all internal libraries

749dd28

PR: icu-units#15 Commit: cc786cf

This was referenced Nov 17, 2020

Revisit MeasureUnitImpl copy(UErrorCode& status) const; #113

Closed

ICU-20568 Add .unit().usage() support to ICU4C NumberFormatter unicode-org/icu#1284

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use `Impl` libraries for all internal libraries #15

Use `Impl` libraries for all internal libraries #15

younies commented Jul 4, 2020

hugovdm left a comment

hugovdm Jul 6, 2020

younies Jul 20, 2020

hugovdm commented Jul 10, 2020

sffc left a comment

sffc left a comment

sffc commented Jul 22, 2020

Use Impl libraries for all internal libraries #15

Use Impl libraries for all internal libraries #15

Conversation

younies commented Jul 4, 2020

Checklist

hugovdm left a comment

Choose a reason for hiding this comment

hugovdm Jul 6, 2020

Choose a reason for hiding this comment

younies Jul 20, 2020

Choose a reason for hiding this comment

hugovdm commented Jul 10, 2020

sffc left a comment

Choose a reason for hiding this comment

sffc left a comment

Choose a reason for hiding this comment

sffc commented Jul 22, 2020

Use `Impl` libraries for all internal libraries #15

Use `Impl` libraries for all internal libraries #15