Update DateIntervalInfo to the new version of ResourceSink (C++)

Update DateIntervalInfo to the new version of ResourceSink (C++)

[fabalbon, 2016-06-30 23:12:46]

https://codereview.appspot.com/296640043/diff/1/source/i18n/dtitvinf.cpp
File source/i18n/dtitvinf.cpp (right):

https://codereview.appspot.com/296640043/diff/1/source/i18n/dtitvinf.cpp#newc...
source/i18n/dtitvinf.cpp:231: DateIntervalInfo &dateIntervalInfo;
This two fields were at the end of this struct.

https://codereview.appspot.com/296640043/diff/1/source/i18n/dtitvinf.cpp#newc...
source/i18n/dtitvinf.cpp:324: UCalendarDateFields
validateAndProcessPatternLetter(const char *patternLetter) {
Same method. Haven't been modified.

https://codereview.appspot.com/296640043/diff/1/source/i18n/dtitvinf.cpp#newc...
source/i18n/dtitvinf.cpp:350: void setIntervalPatternIfAbsent(const char
*currentSkeleton, UCalendarDateFields lrgDiffCalUnit,
currentSkeleton was converted to into a parameter.

[sffc, 2016-06-30 23:53:49]

https://codereview.appspot.com/296640043/diff/1/source/i18n/dtitvinf.cpp
File source/i18n/dtitvinf.cpp (right):

https://codereview.appspot.com/296640043/diff/1/source/i18n/dtitvinf.cpp#newc...
source/i18n/dtitvinf.cpp:292: // Process the key
Check for string like in Java?

if (value.getType() == ICUResourceBundle.STRING) {
    ...
}

https://codereview.appspot.com/296640043/diff/1/source/i18n/dtitvinf.cpp#newc...
source/i18n/dtitvinf.cpp:300: errorCode = U_INVALID_FORMAT_ERROR;
I don't see the place in Java where you throw this exception.

[fabalbon, 2016-07-01 00:43:54]

Update DateIntervalInfo to the new version of ResourceSink (C++)

[fabalbon, 2016-07-01 00:45:56]

https://codereview.appspot.com/296640043/diff/1/source/i18n/dtitvinf.cpp
File source/i18n/dtitvinf.cpp (right):

https://codereview.appspot.com/296640043/diff/1/source/i18n/dtitvinf.cpp#newc...
source/i18n/dtitvinf.cpp:292: // Process the key
On 2016/06/30 23:53:49, sffc wrote:
> Check for string like in Java?
> 
> if (value.getType() == ICUResourceBundle.STRING) {
>     ...
> }

I completely missed this. Thanks!!

https://codereview.appspot.com/296640043/diff/1/source/i18n/dtitvinf.cpp#newc...
source/i18n/dtitvinf.cpp:300: errorCode = U_INVALID_FORMAT_ERROR;
On 2016/06/30 23:53:49, sffc wrote:
> I don't see the place in Java where you throw this exception.

Yes, you are right. This is a discrepancy between the java an c++ versions of
this data loader. Originally there was an exception in java, but it was deleted
because we decided to ignore it instead.

Thanks for catching this!

[sffc, 2016-07-01 20:20:51]

https://codereview.appspot.com/296640043/diff/20001/source/i18n/dtitvinf.cpp
File source/i18n/dtitvinf.cpp (right):

https://codereview.appspot.com/296640043/diff/20001/source/i18n/dtitvinf.cpp#...
source/i18n/dtitvinf.cpp:240: void put(const char *key, ResourceValue &value,
UBool /*noFallback*/, UErrorCode &errorCode) {
Should this be virtual?

[fabalbon, 2016-07-01 22:53:15]

Second round of CR

[fabalbon, 2016-07-01 22:53:26]

https://codereview.appspot.com/296640043/diff/20001/source/i18n/dtitvinf.cpp
File source/i18n/dtitvinf.cpp (right):

https://codereview.appspot.com/296640043/diff/20001/source/i18n/dtitvinf.cpp#...
source/i18n/dtitvinf.cpp:240: void put(const char *key, ResourceValue &value,
UBool /*noFallback*/, UErrorCode &errorCode) {
On 2016/07/01 20:20:51, sffc wrote:
> Should this be virtual?

Yes, it should, thanks!

[sffc, 2016-07-01 22:59:06]

On 2016/07/01 22:53:26, fabalbon wrote:
> https://codereview.appspot.com/296640043/diff/20001/source/i18n/dtitvinf.cpp
> File source/i18n/dtitvinf.cpp (right):
> 
>
https://codereview.appspot.com/296640043/diff/20001/source/i18n/dtitvinf.cpp#...
> source/i18n/dtitvinf.cpp:240: void put(const char *key, ResourceValue &value,
> UBool /*noFallback*/, UErrorCode &errorCode) {
> On 2016/07/01 20:20:51, sffc wrote:
> > Should this be virtual?
> 
> Yes, it should, thanks!

LGTM

Up to you if you want to move the struct to be a private member of the main
class.  Also up to you if you want Andy to check it over for C++ tips.

[fabalbon, 2016-07-06 22:48:19]

Third round of CR

[fabalbon, 2016-07-06 22:51:55]

Hi Andy,

Could you please review the update of DateIntervalInfo to the new version of the
enumeration mechanism?

Shane already reviewed the data loading logic.

Thanks!

[sffc, 2016-07-06 23:19:12]

https://codereview.appspot.com/296640043/diff/60001/source/i18n/dtitvinf.cpp
File source/i18n/dtitvinf.cpp (right):

https://codereview.appspot.com/296640043/diff/60001/source/i18n/dtitvinf.cpp#...
source/i18n/dtitvinf.cpp:353: DateIntervalInfo::IntervalPatternIndex index =
I think there's no need for this 'DateIntervalInfo::' anymore.

[fabalbon, 2016-07-07 14:38:42]

Fourth round of CR

[fabalbon, 2016-07-07 14:39:37]

https://codereview.appspot.com/296640043/diff/60001/source/i18n/dtitvinf.cpp
File source/i18n/dtitvinf.cpp (right):

https://codereview.appspot.com/296640043/diff/60001/source/i18n/dtitvinf.cpp#...
source/i18n/dtitvinf.cpp:353: DateIntervalInfo::IntervalPatternIndex index =
On 2016/07/06 23:19:11, sffc wrote:
> I think there's no need for this 'DateIntervalInfo::' anymore.

Done.

[andy.heninger, 2016-07-18 20:03:12]

LGTM