Issue 295240043: DateTimePattern skeleton string optimization - C++ (Closed)
|
Created:
7 years, 11 months ago by sffc Modified:
7 years, 11 months ago Reviewers:
markus.icu CC:
fabalbon Base URL:
svn+icussh://source.icu-project.org/repos/icu/icu/trunk/source/ Visibility:
Public. |
DescriptionDateTimePattern skeleton string optimization - C++
Patch Set 1 #Patch Set 2 : Updating with feedback form Java version. #
Total comments: 56
Patch Set 3 : Updating with Markus's first round of feedback. #
Total comments: 17
Patch Set 4 : Updating with Markus's second round of feedback. #
MessagesTotal messages: 14
This change reduces the memory consumption of PatternMap (inside DateTimePatternGenerator) from 86 KB to 8 KB. Heap profile from old code: ->39.91% (86,920B) 0x5144332: icu_58::PatternMap::add(icu_58::UnicodeString const&, icu_58::PtnSkeleton const&, icu_58::UnicodeString const&, signed char, UErrorCode&) (dtptngen.cpp:1670) Heap profile from new code: ->09.46% (8,760B) 0x5142DC6: icu_58::PatternMap::add(icu_58::UnicodeString const&, icu_58::PtnSkeleton const&, icu_58::UnicodeString const&, signed char, UErrorCode&) (dtptngen.cpp:1655)
Sign in to reply to this message.
Updating with feedback form Java version.
Sign in to reply to this message.
Updating C++ data structures for Java consistency. https://codereview.appspot.com/295240043/diff/20001/i18n/dtptngen.cpp File i18n/dtptngen.cpp (right): https://codereview.appspot.com/295240043/diff/20001/i18n/dtptngen.cpp#newcode... i18n/dtptngen.cpp:1067: if (Canonical_Items[i] > 0) { Fixing small bug. The last element of Canonical_Items is the null byte, and it was triggering a U_INVALID_CHARACTER deeper in the code. The exception was being masked because this function didn't bubble the exception up. https://codereview.appspot.com/295240043/diff/20001/i18n/unicode/dtptngen.h File i18n/unicode/dtptngen.h (right): https://codereview.appspot.com/295240043/diff/20001/i18n/unicode/dtptngen.h#n... i18n/unicode/dtptngen.h:537: void addCanonicalItems(UErrorCode &status); I noticed that addCanonicalItems was previously masking exceptions. I made this change to make exceptions bubble up. This is in the private section of the class, so it should be harmless.
Sign in to reply to this message.
Incorporating the changes in Java to C++.
Sign in to reply to this message.
https://codereview.appspot.com/295240043/diff/20001/i18n/dtptngen.cpp File i18n/dtptngen.cpp (right): https://codereview.appspot.com/295240043/diff/20001/i18n/dtptngen.cpp#newcode424 i18n/dtptngen.cpp:424: if (U_FAILURE(status)) { return; } remove this line, the next one checks for status too https://codereview.appspot.com/295240043/diff/20001/i18n/dtptngen.cpp#newcode426 i18n/dtptngen.cpp:426: if (U_FAILURE(status)) { return; } remove this too -- make sure that each of the functions called from here check the status before they do anything; with the exception of calling yet more functions that take&check status https://codereview.appspot.com/295240043/diff/20001/i18n/dtptngen.cpp#newcode... i18n/dtptngen.cpp:1068: addPattern(UnicodeString(Canonical_Items[i]), FALSE, conflictingPattern, status); What is the type of Canonical_Items[i]? char? UChar? const char *? https://codereview.appspot.com/295240043/diff/20001/i18n/dtptngen.cpp#newcode... i18n/dtptngen.cpp:1713: if (curElem->skeleton->original.compareField(i, skeleton.original) != 0 ) Compare the whole original objects rather than loop over the fields? Is that what you do in Java now? https://codereview.appspot.com/295240043/diff/20001/i18n/dtptngen.cpp#newcode... i18n/dtptngen.cpp:1720: if (curElem->skeleton->baseOriginal.compareField(i, skeleton.baseOriginal) != 0 ) ditto https://codereview.appspot.com/295240043/diff/20001/i18n/dtptngen.cpp#newcode... i18n/dtptngen.cpp:1834: UnicodeString value = fp->items[i]; Please try to make this a const UnicodeString &value to avoid making an object and copying its contents. It feels like this was ported to C++ by someone not familiar with C++ ... :-/ https://codereview.appspot.com/295240043/diff/20001/i18n/dtptngen.cpp#newcode... i18n/dtptngen.cpp:1869: UnicodeString& No, you must return this by value. Otherwise you are returning a pointer to a temporary object on the stack of the function call. At least, the compiler should use https://en.wikipedia.org/wiki/Return_value_optimization https://codereview.appspot.com/295240043/diff/20001/i18n/dtptngen.cpp#newcode... i18n/dtptngen.cpp:1905: skeleton.copyFrom(newSkeleton); In C++, this should use the assignment operator, rather than a "copyFrom" function. https://codereview.appspot.com/295240043/diff/20001/i18n/dtptngen.cpp#newcode... i18n/dtptngen.cpp:2205: UChar ch(chars[field]); Optional: If you use uint8_t as the unit type for chars, you should not need to explicitly cast to UChar. https://codereview.appspot.com/295240043/diff/20001/i18n/dtptngen.cpp#newcode... i18n/dtptngen.cpp:2210: return (int32_t) lengths[field]; No need for explicit cast from int8_t to int32_t. https://codereview.appspot.com/295240043/diff/20001/i18n/dtptngen.cpp#newcode... i18n/dtptngen.cpp:2213: void SkeletonFields::populate(int32_t field, UnicodeString value) { const UnicodeString &value https://codereview.appspot.com/295240043/diff/20001/i18n/dtptngen.cpp#newcode... i18n/dtptngen.cpp:2226: UnicodeString& SkeletonFields::toString() const { You must return this by value. https://codereview.appspot.com/295240043/diff/20001/i18n/dtptngen.cpp#newcode... i18n/dtptngen.cpp:2280: void PtnSkeleton::copyFrom(const PtnSkeleton& other) { This should be operator== https://codereview.appspot.com/295240043/diff/20001/i18n/dtptngen.cpp#newcode... i18n/dtptngen.cpp:2282: type[i] = other.type[i]; uprv_memcpy, no loop? https://codereview.appspot.com/295240043/diff/20001/i18n/dtptngen.cpp#newcode... i18n/dtptngen.cpp:2283: original.copyFieldFrom(i, other.original); original = other.original; baseOriginal = other.baseOriginal; ... outside the loop https://codereview.appspot.com/295240043/diff/20001/i18n/dtptngen.cpp#newcode... i18n/dtptngen.cpp:2301: UnicodeString& You must return this by value. https://codereview.appspot.com/295240043/diff/20001/i18n/dtptngen.cpp#newcode... i18n/dtptngen.cpp:2307: UnicodeString& ditto https://codereview.appspot.com/295240043/diff/20001/i18n/dtptngen_impl.h File i18n/dtptngen_impl.h (right): https://codereview.appspot.com/295240043/diff/20001/i18n/dtptngen_impl.h#newc... i18n/dtptngen_impl.h:116: // It's important that this class have a small memory footprint because over a thousand instances "over a thousand instances" -> it's only some dozens now with the multi-field structure, right? I think you changed this comment in Java while modifying the class. Please make sure C++ is really parallel with Java. https://codereview.appspot.com/295240043/diff/20001/i18n/dtptngen_impl.h#newc... i18n/dtptngen_impl.h:126: void populate(int32_t field, UnicodeString value); const UnicodeString &value https://codereview.appspot.com/295240043/diff/20001/i18n/dtptngen_impl.h#newc... i18n/dtptngen_impl.h:129: UnicodeString& toString() const; If this builds a new string object, then you must return it by value, not by reference. https://codereview.appspot.com/295240043/diff/20001/i18n/dtptngen_impl.h#newc... i18n/dtptngen_impl.h:133: int8_t compareField(int32_t field, const SkeletonFields& other) const; In Java you changed to compare whole SkeletonFields, not single fields, right? https://codereview.appspot.com/295240043/diff/20001/i18n/dtptngen_impl.h#newc... i18n/dtptngen_impl.h:136: virtual ~SkeletonFields(); Please make this non-virtual and move it to just after the constructor. If it's empty, you can omit it. https://codereview.appspot.com/295240043/diff/20001/i18n/dtptngen_impl.h#newc... i18n/dtptngen_impl.h:166: virtual ~PtnSkeleton(); Why is this virtual? (I see it's predates your changes...) We don't want vtables when we don't use class objects polymorphically. When we do need virtual functions, then we extend UObject, not UMemory. For consistency, destructors should be declared right after constructors. For virtual destructors, we require it.
Sign in to reply to this message.
Updating with Markus's first round of feedback.
Sign in to reply to this message.
Updating with Markus's first round of feedback.
Sign in to reply to this message.
C++ isn't my first language. :) https://codereview.appspot.com/295240043/diff/20001/i18n/dtptngen.cpp File i18n/dtptngen.cpp (right): https://codereview.appspot.com/295240043/diff/20001/i18n/dtptngen.cpp#newcode424 i18n/dtptngen.cpp:424: if (U_FAILURE(status)) { return; } On 2016/05/27 23:24:06, markus.icu wrote: > remove this line, the next one checks for status too Done. https://codereview.appspot.com/295240043/diff/20001/i18n/dtptngen.cpp#newcode426 i18n/dtptngen.cpp:426: if (U_FAILURE(status)) { return; } On 2016/05/27 23:24:06, markus.icu wrote: > remove this too -- make sure that each of the functions called from here check > the status before they do anything; with the exception of calling yet more > functions that take&check status Done. https://codereview.appspot.com/295240043/diff/20001/i18n/dtptngen.cpp#newcode... i18n/dtptngen.cpp:1068: addPattern(UnicodeString(Canonical_Items[i]), FALSE, conflictingPattern, status); On 2016/05/27 23:24:06, markus.icu wrote: > What is the type of Canonical_Items[i]? char? UChar? const char *? Canonical_Items is a static const UChar[] with 16 items. I just changed it to a static const char[] and it still compiles and the tests still pass. https://codereview.appspot.com/295240043/diff/20001/i18n/dtptngen.cpp#newcode... i18n/dtptngen.cpp:1713: if (curElem->skeleton->original.compareField(i, skeleton.original) != 0 ) On 2016/05/27 23:24:06, markus.icu wrote: > Compare the whole original objects rather than loop over the fields? Is that > what you do in Java now? I can use operator== and simplify this function a great deal. Thanks. This particular function doesn't have an analog in Java. Java uses a different data structure (a Map) to look up patterns from skeletons. https://codereview.appspot.com/295240043/diff/20001/i18n/dtptngen.cpp#newcode... i18n/dtptngen.cpp:1720: if (curElem->skeleton->baseOriginal.compareField(i, skeleton.baseOriginal) != 0 ) On 2016/05/27 23:24:06, markus.icu wrote: > ditto Done. https://codereview.appspot.com/295240043/diff/20001/i18n/dtptngen.cpp#newcode... i18n/dtptngen.cpp:1834: UnicodeString value = fp->items[i]; On 2016/05/27 23:24:06, markus.icu wrote: > Please try to make this a > const UnicodeString &value > > to avoid making an object and copying its contents. > > It feels like this was ported to C++ by someone not familiar with C++ ... :-/ Done. https://codereview.appspot.com/295240043/diff/20001/i18n/dtptngen.cpp#newcode... i18n/dtptngen.cpp:1869: UnicodeString& On 2016/05/27 23:24:06, markus.icu wrote: > No, you must return this by value. Otherwise you are returning a pointer to a > temporary object on the stack of the function call. > > At least, the compiler should use > https://en.wikipedia.org/wiki/Return_value_optimization Done, thanks. C++ isn't my best language. https://codereview.appspot.com/295240043/diff/20001/i18n/dtptngen.cpp#newcode... i18n/dtptngen.cpp:1905: skeleton.copyFrom(newSkeleton); On 2016/05/27 23:24:06, markus.icu wrote: > In C++, this should use the assignment operator, rather than a "copyFrom" > function. There are multiple classes in this file that define and use a "copyFrom" method. I did this to be consistent with the rest of the file. Would you like me to go and change the other methods from "copyFrom" to "operator=" ? https://codereview.appspot.com/295240043/diff/20001/i18n/dtptngen.cpp#newcode... i18n/dtptngen.cpp:2205: UChar ch(chars[field]); On 2016/05/27 23:24:06, markus.icu wrote: > Optional: If you use uint8_t as the unit type for chars, you should not need to > explicitly cast to UChar. Done. https://codereview.appspot.com/295240043/diff/20001/i18n/dtptngen.cpp#newcode... i18n/dtptngen.cpp:2210: return (int32_t) lengths[field]; On 2016/05/27 23:24:06, markus.icu wrote: > No need for explicit cast from int8_t to int32_t. Done. https://codereview.appspot.com/295240043/diff/20001/i18n/dtptngen.cpp#newcode... i18n/dtptngen.cpp:2213: void SkeletonFields::populate(int32_t field, UnicodeString value) { On 2016/05/27 23:24:06, markus.icu wrote: > const UnicodeString &value Done. https://codereview.appspot.com/295240043/diff/20001/i18n/dtptngen.cpp#newcode... i18n/dtptngen.cpp:2226: UnicodeString& SkeletonFields::toString() const { On 2016/05/27 23:24:06, markus.icu wrote: > You must return this by value. Done. https://codereview.appspot.com/295240043/diff/20001/i18n/dtptngen.cpp#newcode... i18n/dtptngen.cpp:2280: void PtnSkeleton::copyFrom(const PtnSkeleton& other) { On 2016/05/27 23:24:06, markus.icu wrote: > This should be operator== See above https://codereview.appspot.com/295240043/diff/20001/i18n/dtptngen.cpp#newcode... i18n/dtptngen.cpp:2282: type[i] = other.type[i]; On 2016/05/27 23:24:06, markus.icu wrote: > uprv_memcpy, no loop? Done. https://codereview.appspot.com/295240043/diff/20001/i18n/dtptngen.cpp#newcode... i18n/dtptngen.cpp:2283: original.copyFieldFrom(i, other.original); On 2016/05/27 23:24:06, markus.icu wrote: > original = other.original; > baseOriginal = other.baseOriginal; > > ... outside the loop See above https://codereview.appspot.com/295240043/diff/20001/i18n/dtptngen.cpp#newcode... i18n/dtptngen.cpp:2301: UnicodeString& On 2016/05/27 23:24:06, markus.icu wrote: > You must return this by value. Done. https://codereview.appspot.com/295240043/diff/20001/i18n/dtptngen.cpp#newcode... i18n/dtptngen.cpp:2307: UnicodeString& On 2016/05/27 23:24:06, markus.icu wrote: > ditto Done. https://codereview.appspot.com/295240043/diff/20001/i18n/dtptngen_impl.h File i18n/dtptngen_impl.h (right): https://codereview.appspot.com/295240043/diff/20001/i18n/dtptngen_impl.h#newc... i18n/dtptngen_impl.h:116: // It's important that this class have a small memory footprint because over a thousand instances On 2016/05/27 23:24:07, markus.icu wrote: > "over a thousand instances" -> it's only some dozens now with the multi-field > structure, right? I think you changed this comment in Java while modifying the > class. Please make sure C++ is really parallel with Java. Yes, you're right. I changed the comment in Java but not here. Fixed. https://codereview.appspot.com/295240043/diff/20001/i18n/dtptngen_impl.h#newc... i18n/dtptngen_impl.h:126: void populate(int32_t field, UnicodeString value); On 2016/05/27 23:24:07, markus.icu wrote: > const UnicodeString &value Done. https://codereview.appspot.com/295240043/diff/20001/i18n/dtptngen_impl.h#newc... i18n/dtptngen_impl.h:129: UnicodeString& toString() const; On 2016/05/27 23:24:07, markus.icu wrote: > If this builds a new string object, then you must return it by value, not by > reference. Done. https://codereview.appspot.com/295240043/diff/20001/i18n/dtptngen_impl.h#newc... i18n/dtptngen_impl.h:133: int8_t compareField(int32_t field, const SkeletonFields& other) const; On 2016/05/27 23:24:06, markus.icu wrote: > In Java you changed to compare whole SkeletonFields, not single fields, right? Turns out that this method isn't necessary in C++. I removed it. https://codereview.appspot.com/295240043/diff/20001/i18n/dtptngen_impl.h#newc... i18n/dtptngen_impl.h:136: virtual ~SkeletonFields(); On 2016/05/27 23:24:06, markus.icu wrote: > Please make this non-virtual and move it to just after the constructor. > If it's empty, you can omit it. Removed it. https://codereview.appspot.com/295240043/diff/20001/i18n/dtptngen_impl.h#newc... i18n/dtptngen_impl.h:166: virtual ~PtnSkeleton(); On 2016/05/27 23:24:07, markus.icu wrote: > Why is this virtual? (I see it's predates your changes...) > > We don't want vtables when we don't use class objects polymorphically. > > When we do need virtual functions, then we extend UObject, not UMemory. > > For consistency, destructors should be declared right after constructors. For > virtual destructors, we require it. I don't know why it's there. Probably someone like me who doesn't know C++ very well put it there while porting. Most of the classes in this file have virtual destructors that are empty. However, when I tried removing the virtual destructors or make them non-virtual, I got the following compiler warning: warning: delete called on 'icu_58::FormatParser' that has virtual functions but non-virtual destructor [-Wdelete-non-virtual-dtor] FormatParser is like the other classes in that it simply inherits from UMemory. I changed them back to make sure I don't screw something up. Let me know what I should do. https://codereview.appspot.com/295240043/diff/20001/i18n/unicode/dtptngen.h File i18n/unicode/dtptngen.h (right): https://codereview.appspot.com/295240043/diff/20001/i18n/unicode/dtptngen.h#n... i18n/unicode/dtptngen.h:521: UnicodeString hackPattern; Removing this field and passing it down via return values (like Java). Sorry to mix it in with the other changes.
Sign in to reply to this message.
https://codereview.appspot.com/295240043/diff/20001/i18n/dtptngen.cpp File i18n/dtptngen.cpp (right): https://codereview.appspot.com/295240043/diff/20001/i18n/dtptngen.cpp#newcode... i18n/dtptngen.cpp:1068: addPattern(UnicodeString(Canonical_Items[i]), FALSE, conflictingPattern, status); On 2016/05/28 01:34:47, sffc wrote: > Canonical_Items is a static const UChar[] with 16 items. I just changed it to a > static const char[] and it still compiles and the tests still pass. I think it's much cleaner to keep it as a UChar[]. It's small and not on the heap, so harmless. Thanks for checking on the type. Please make a note to look at whether addPattern could be changed (or have another version) so that we don't have to turn a UChar into a UnicodeString. https://codereview.appspot.com/295240043/diff/20001/i18n/dtptngen.cpp#newcode... i18n/dtptngen.cpp:1869: UnicodeString& On 2016/05/28 01:34:47, sffc wrote: > Done, thanks. C++ isn't my best language. You are doing fine getting the hang of it :-) https://codereview.appspot.com/295240043/diff/20001/i18n/dtptngen.cpp#newcode... i18n/dtptngen.cpp:1905: skeleton.copyFrom(newSkeleton); On 2016/05/28 01:34:47, sffc wrote: > There are multiple classes in this file that define and use a "copyFrom" method. > I did this to be consistent with the rest of the file. Would you like me to go > and change the other methods from "copyFrom" to "operator=" ? Yes please, because it's good to clean this up (this code seems particularly messy and weird), but it's probably best to make a note of that and do it later. https://codereview.appspot.com/295240043/diff/20001/i18n/dtptngen_impl.h File i18n/dtptngen_impl.h (right): https://codereview.appspot.com/295240043/diff/20001/i18n/dtptngen_impl.h#newc... i18n/dtptngen_impl.h:166: virtual ~PtnSkeleton(); On 2016/05/28 01:34:48, sffc wrote: > Most of the classes in this file have virtual destructors that are empty. > However, when I tried removing the virtual destructors or make them non-virtual, > I got the following compiler warning: > > warning: delete called on 'icu_58::FormatParser' that has virtual functions but > non-virtual destructor [-Wdelete-non-virtual-dtor] > > FormatParser is like the other classes in that it simply inherits from UMemory. > I changed them back to make sure I don't screw something up. Let me know what I > should do. Please make a note, and let's revisit this after the skeleton change and after the resource sink changes. https://codereview.appspot.com/295240043/diff/60001/i18n/dtptngen.cpp File i18n/dtptngen.cpp (right): https://codereview.appspot.com/295240043/diff/60001/i18n/dtptngen.cpp#newcode134 i18n/dtptngen.cpp:134: static const char Canonical_Items[] = { Please change it back to UChar -- see below. https://codereview.appspot.com/295240043/diff/60001/i18n/dtptngen.cpp#newcode424 i18n/dtptngen.cpp:424: UnicodeString hackPattern = addICUPatterns(locale, status); What's up with hackPattern? Is it a temporary debugging thing? It looks like it's always empty. https://codereview.appspot.com/295240043/diff/60001/i18n/dtptngen.cpp#newcode431 i18n/dtptngen.cpp:431: if (hackPattern.length()>0) { if (!hackPattern.isEmpty()) { https://codereview.appspot.com/295240043/diff/60001/i18n/dtptngen.cpp#newcode... i18n/dtptngen.cpp:1091: if (U_FAILURE(status)) { return; } We technically don't need this one at all. I think it would be useful, but just after the Calendar::createInstance(locale, status) rather than here just before it. Please move it. https://codereview.appspot.com/295240043/diff/60001/i18n/dtptngen.cpp#newcode... i18n/dtptngen.cpp:1113: if (U_FAILURE(status)) { return; } unnecessary -- the following constructor takes status https://codereview.appspot.com/295240043/diff/60001/i18n/dtptngen.cpp#newcode... i18n/dtptngen.cpp:2222: return appendTo(string); Actually, don't return the result of appendTo(). The compiler might not see that it's the same as the local string variable, and use a copy constructor rather than returning string itself. UnicodeString string; appendTo(string); return string; ... and please look at whether toString() is even called in C++ ... https://codereview.appspot.com/295240043/diff/60001/i18n/dtptngen_impl.h File i18n/dtptngen_impl.h (right): https://codereview.appspot.com/295240043/diff/60001/i18n/dtptngen_impl.h#newc... i18n/dtptngen_impl.h:128: UnicodeString toString() const; Thanks for fixing the return type. Is this actually called in C++, or is it over-ported from Java?
Sign in to reply to this message.
https://codereview.appspot.com/295240043/diff/60001/i18n/dtptngen.cpp File i18n/dtptngen.cpp (right): https://codereview.appspot.com/295240043/diff/60001/i18n/dtptngen.cpp#newcode424 i18n/dtptngen.cpp:424: UnicodeString hackPattern = addICUPatterns(locale, status); On 2016/05/28 18:15:39, markus.icu wrote: > What's up with hackPattern? Is it a temporary debugging thing? It looks like > it's always empty. Sorry, I didn't see it getting set when I just looked at diffs from ps2. After looking at the Java sink changes, I see what's going on. Please s/hackPattern/shortTimePattern/ to make it consistent with Java, and less, well, hack-y...
Sign in to reply to this message.
Updating with Markus's second round of feedback.
Sign in to reply to this message.
https://codereview.appspot.com/295240043/diff/20001/i18n/dtptngen.cpp File i18n/dtptngen.cpp (right): https://codereview.appspot.com/295240043/diff/20001/i18n/dtptngen.cpp#newcode... i18n/dtptngen.cpp:1068: addPattern(UnicodeString(Canonical_Items[i]), FALSE, conflictingPattern, status); On 2016/05/28 18:15:38, markus.icu wrote: > On 2016/05/28 01:34:47, sffc wrote: > > Canonical_Items is a static const UChar[] with 16 items. I just changed it to > a > > static const char[] and it still compiles and the tests still pass. > > I think it's much cleaner to keep it as a UChar[]. It's small and not on the > heap, so harmless. > > Thanks for checking on the type. Please make a note to look at whether > addPattern could be changed (or have another version) so that we don't have to > turn a UChar into a UnicodeString. The UnicodeString gets passed down through the stack a few times until it reaches FormatParser::set(), which tokenizes the string. This whole call stack could be optimized, but I think it's a rather big change for a small reward. This particular call site runs only a few times (16 to be exact) per data load. https://codereview.appspot.com/295240043/diff/20001/i18n/dtptngen.cpp#newcode... i18n/dtptngen.cpp:1905: skeleton.copyFrom(newSkeleton); On 2016/05/28 18:15:39, markus.icu wrote: > On 2016/05/28 01:34:47, sffc wrote: > > There are multiple classes in this file that define and use a "copyFrom" > method. > > I did this to be consistent with the rest of the file. Would you like me to > go > > and change the other methods from "copyFrom" to "operator=" ? > > Yes please, because it's good to clean this up (this code seems particularly > messy and weird), but it's probably best to make a note of that and do it later. I'll go through and make this change en-masse in a separate commit after this one. https://codereview.appspot.com/295240043/diff/20001/i18n/dtptngen_impl.h File i18n/dtptngen_impl.h (right): https://codereview.appspot.com/295240043/diff/20001/i18n/dtptngen_impl.h#newc... i18n/dtptngen_impl.h:166: virtual ~PtnSkeleton(); On 2016/05/28 18:15:39, markus.icu wrote: > On 2016/05/28 01:34:48, sffc wrote: > > Most of the classes in this file have virtual destructors that are empty. > > However, when I tried removing the virtual destructors or make them > non-virtual, > > I got the following compiler warning: > > > > warning: delete called on 'icu_58::FormatParser' that has virtual functions > but > > non-virtual destructor [-Wdelete-non-virtual-dtor] > > > > FormatParser is like the other classes in that it simply inherits from > UMemory. > > I changed them back to make sure I don't screw something up. Let me know what > I > > should do. > > Please make a note, and let's revisit this after the skeleton change and after > the resource sink changes. Note added. https://codereview.appspot.com/295240043/diff/60001/i18n/dtptngen.cpp File i18n/dtptngen.cpp (right): https://codereview.appspot.com/295240043/diff/60001/i18n/dtptngen.cpp#newcode134 i18n/dtptngen.cpp:134: static const char Canonical_Items[] = { On 2016/05/28 18:15:39, markus.icu wrote: > Please change it back to UChar -- see below. Done. https://codereview.appspot.com/295240043/diff/60001/i18n/dtptngen.cpp#newcode424 i18n/dtptngen.cpp:424: UnicodeString hackPattern = addICUPatterns(locale, status); On 2016/05/28 18:52:02, markus.icu wrote: > On 2016/05/28 18:15:39, markus.icu wrote: > > What's up with hackPattern? Is it a temporary debugging thing? It looks like > > it's always empty. > > Sorry, I didn't see it getting set when I just looked at diffs from ps2. After > looking at the Java sink changes, I see what's going on. > > Please s/hackPattern/shortTimePattern/ to make it consistent with Java, and > less, well, hack-y... In C++, it's a different string that gets saved as the "hackPattern". C++ uses the DateFormat::kMedium pattern, but Java uses the DateFormat.SHORT pattern. Who's right? https://codereview.appspot.com/295240043/diff/60001/i18n/dtptngen.cpp#newcode431 i18n/dtptngen.cpp:431: if (hackPattern.length()>0) { On 2016/05/28 18:15:39, markus.icu wrote: > if (!hackPattern.isEmpty()) { Done. https://codereview.appspot.com/295240043/diff/60001/i18n/dtptngen.cpp#newcode... i18n/dtptngen.cpp:1091: if (U_FAILURE(status)) { return; } On 2016/05/28 18:15:39, markus.icu wrote: > We technically don't need this one at all. I think it would be useful, but just > after the Calendar::createInstance(locale, status) rather than here just before > it. Please move it. Done. https://codereview.appspot.com/295240043/diff/60001/i18n/dtptngen.cpp#newcode... i18n/dtptngen.cpp:1113: if (U_FAILURE(status)) { return; } On 2016/05/28 18:15:39, markus.icu wrote: > unnecessary -- the following constructor takes status Done. https://codereview.appspot.com/295240043/diff/60001/i18n/dtptngen.cpp#newcode... i18n/dtptngen.cpp:2222: return appendTo(string); On 2016/05/28 18:15:39, markus.icu wrote: > Actually, don't return the result of appendTo(). The compiler might not see that > it's the same as the local string variable, and use a copy constructor rather > than returning string itself. > > UnicodeString string; > appendTo(string); > return string; > > ... and please look at whether toString() is even called in C++ ... Indeed, toString() is never called. I removed it. https://codereview.appspot.com/295240043/diff/60001/i18n/dtptngen_impl.h File i18n/dtptngen_impl.h (right): https://codereview.appspot.com/295240043/diff/60001/i18n/dtptngen_impl.h#newc... i18n/dtptngen_impl.h:128: UnicodeString toString() const; On 2016/05/28 18:15:39, markus.icu wrote: > Thanks for fixing the return type. Is this actually called in C++, or is it > over-ported from Java? Yup, it's never used. Removed.
Sign in to reply to this message.
LGTM https://codereview.appspot.com/295240043/diff/60001/i18n/dtptngen.cpp File i18n/dtptngen.cpp (right): https://codereview.appspot.com/295240043/diff/60001/i18n/dtptngen.cpp#newcode424 i18n/dtptngen.cpp:424: UnicodeString hackPattern = addICUPatterns(locale, status); On 2016/05/31 22:34:37, sffc wrote: > On 2016/05/28 18:52:02, markus.icu wrote: > > On 2016/05/28 18:15:39, markus.icu wrote: > > > What's up with hackPattern? Is it a temporary debugging thing? It looks like > > > it's always empty. > > > > Sorry, I didn't see it getting set when I just looked at diffs from ps2. After > > looking at the Java sink changes, I see what's going on. > > > > Please s/hackPattern/shortTimePattern/ to make it consistent with Java, > and > > less, well, hack-y... > > In C++, it's a different string that gets saved as the "hackPattern". C++ uses > the DateFormat::kMedium pattern, but Java uses the DateFormat.SHORT pattern. > Who's right? I don't know who is right, but I am sure that they should behave the same. Please submit an ICU ticket for this.
Sign in to reply to this message.
Submitted as r38780. https://codereview.appspot.com/295240043/diff/60001/i18n/dtptngen.cpp File i18n/dtptngen.cpp (right): https://codereview.appspot.com/295240043/diff/60001/i18n/dtptngen.cpp#newcode424 i18n/dtptngen.cpp:424: UnicodeString hackPattern = addICUPatterns(locale, status); On 2016/05/31 22:47:10, markus.icu wrote: > On 2016/05/31 22:34:37, sffc wrote: > > On 2016/05/28 18:52:02, markus.icu wrote: > > > On 2016/05/28 18:15:39, markus.icu wrote: > > > > What's up with hackPattern? Is it a temporary debugging thing? It looks > like > > > > it's always empty. > > > > > > Sorry, I didn't see it getting set when I just looked at diffs from ps2. > After > > > looking at the Java sink changes, I see what's going on. > > > > > > Please s/hackPattern/shortTimePattern/ to make it consistent with Java, > > and > > > less, well, hack-y... > > > > In C++, it's a different string that gets saved as the "hackPattern". C++ > uses > > the DateFormat::kMedium pattern, but Java uses the DateFormat.SHORT pattern. > > Who's right? > > I don't know who is right, but I am sure that they should behave the same. > Please submit an ICU ticket for this. I checked the SVN history, and in C++, the "hackPattern" thing originates all the way back from the original commit of this file, in revision 22029 (7/18/2007). In Java, this variable was originally named "hackPattern" and used the MEDIUM format pattern, but in revision 25662 (ticket:6800, 3/26/2009), yoshito changed the name of the variable and made it use the SHORT format pattern instead. Appears he may have missed it in C++. Make a ticket, or change C++ to use SHORT?
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
