Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(599)

Issue 339290043: Adding currency names matcher to C++ number parsing code.

Can't Edit
Can't Publish+Mail
Start Review
Created:
6 months, 1 week ago by sffc
Modified:
6 months, 1 week ago
Reviewers:
andy.heninger
Base URL:
svn+icussh://source.icu-project.org/repos/icu/branches/shane/numberformat4/
Visibility:
Public.

Description

Adding currency names matcher to C++ number parsing code.

Patch Set 1 #

Total comments: 8
Unified diffs Side-by-side diffs Delta from patch set Stats (+257 lines, -40 lines) Patch
M icu4c/source/common/ucurr.cpp View 13 chunks +94 lines, -31 lines 2 comments Download
M icu4c/source/common/ucurrimp.h View 3 chunks +15 lines, -0 lines 2 comments Download
M icu4c/source/i18n/Makefile.in View 1 chunk +2 lines, -1 line 0 comments Download
M icu4c/source/i18n/decimfmt.cpp View 1 chunk +2 lines, -1 line 0 comments Download
A icu4c/source/i18n/numparse_currency.h View 1 chunk +49 lines, -0 lines 2 comments Download
A icu4c/source/i18n/numparse_currency.cpp View 1 chunk +70 lines, -0 lines 0 comments Download
M icu4c/source/i18n/numparse_impl.h View 2 chunks +4 lines, -0 lines 2 comments Download
M icu4c/source/i18n/numparse_impl.cpp View 2 chunks +16 lines, -2 lines 0 comments Download
M icu4c/source/i18n/numparse_parsednumber.cpp View 1 chunk +1 line, -1 line 0 comments Download
M icu4c/source/i18n/numparse_types.h View 1 chunk +1 line, -1 line 0 comments Download
M icu4c/source/test/intltest/numbertest_parse.cpp View 2 chunks +3 lines, -3 lines 0 comments Download

Messages

Total messages: 3
sffc
Hi Andy, I'd like you to review this chunk of my parsing code. This is ...
6 months, 1 week ago (2018-02-09 10:59:38 UTC) #1
andy.heninger
https://codereview.appspot.com/339290043/diff/1/icu4c/source/common/ucurr.cpp File icu4c/source/common/ucurr.cpp (right): https://codereview.appspot.com/339290043/diff/1/icu4c/source/common/ucurr.cpp#newcode1307 icu4c/source/common/ucurr.cpp:1307: // Check for partial matches. Will this match half ...
6 months, 1 week ago (2018-02-10 01:26:44 UTC) #2
sffc
6 months, 1 week ago (2018-02-10 03:00:05 UTC) #3
Committed as revision 40889.

https://codereview.appspot.com/339290043/diff/1/icu4c/source/common/ucurr.cpp
File icu4c/source/common/ucurr.cpp (right):

https://codereview.appspot.com/339290043/diff/1/icu4c/source/common/ucurr.cpp...
icu4c/source/common/ucurr.cpp:1307: // Check for partial matches.
On 2018/02/10 01:26:44, andy.heninger wrote:
> Will this match half of a supplemental character?

Hmm, yes, it would, but I don't see how it would be an issue since strings
aren't accepted unless there is a complete match.

https://codereview.appspot.com/339290043/diff/1/icu4c/source/common/ucurrimp.h
File icu4c/source/common/ucurrimp.h (right):

https://codereview.appspot.com/339290043/diff/1/icu4c/source/common/ucurrimp....
icu4c/source/common/ucurrimp.h:48: * this may be nonzero even if no full
currency was matched.
On 2018/02/10 01:26:44, andy.heninger wrote:
> Is this allowed to be null?

No.  Added to the docstring.

https://codereview.appspot.com/339290043/diff/1/icu4c/source/i18n/numparse_cu...
File icu4c/source/i18n/numparse_currency.h (right):

https://codereview.appspot.com/339290043/diff/1/icu4c/source/i18n/numparse_cu...
icu4c/source/i18n/numparse_currency.h:26: CurrencyNamesMatcher() = default;  //
WARNING: Leaves the object in an unusable state
On 2018/02/10 01:26:44, andy.heninger wrote:
> Needs to leave it in good enough shape for the destructor to run safely.

The CharString is default-initialized also, so it should be okay.

https://codereview.appspot.com/339290043/diff/1/icu4c/source/i18n/numparse_im...
File icu4c/source/i18n/numparse_impl.h (right):

https://codereview.appspot.com/339290043/diff/1/icu4c/source/i18n/numparse_im...
icu4c/source/i18n/numparse_impl.h:47: // WARNING: All of these matchers start in
an uninitialized state.
On 2018/02/10 01:26:44, andy.heninger wrote:
> Default initialized is different from uninitialized. Some constructor always
> runs, and needs to get everything in sufficiently good shape that destructors
> are safe. Uninitialized sounds like naked raw memory, which it is not.

Done.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b