LocaleDisplayNames - C++

LocaleDisplayNames C++

[sffc, 2016-05-17 02:41:01]

LocaleDisplayNames C++

[markus.icu, 2016-05-30 18:48:22]

https://codereview.appspot.com/300750043/diff/20001/common/locdspnm.cpp
File common/locdspnm.cpp (right):

https://codereview.appspot.com/300750043/diff/20001/common/locdspnm.cpp#newco...
common/locdspnm.cpp:272: namespace CapitalizationContextSinkNamespace {
Optional: In google3, this is often called "namespace internal".

Optional alternative: You could make CapitalizationContextSink a struct nested
inside LocaleDisplayNamesImpl, more like in Java, and then you don't need the
namespace and hopefully don't need to qualify things like
LocaleDisplayNamesImpl::kCapContextUsageScript. You might still need the friend
declaration.

https://codereview.appspot.com/300750043/diff/20001/common/locdspnm.cpp#newco...
common/locdspnm.cpp:273: struct CapitalizationContextSink;
Please indent by 4 each level; check throughout the file.

https://codereview.appspot.com/300750043/diff/20001/common/locdspnm.cpp#newco...
common/locdspnm.cpp:411: #define CHECK_KEY_FOR_USAGE_ENUM(name, value) if
(uprv_strcmp(key, name) == 0) { usageEnum = value; }
Please don't use a macro for this, it makes for very hacky code.

https://codereview.appspot.com/300750043/diff/20001/common/locdspnm.cpp#newco...
common/locdspnm.cpp:491: // Silently ignore.  Not every locale has
contextTransforms.
Don't you need to reset the status?

[sffc, 2016-05-31 21:29:34]

Updating with Markus's first round of feedback.

[sffc, 2016-05-31 21:36:21]

Changing from 'internal' namespace to a shared namespace.

[sffc, 2016-05-31 21:38:05]

Changing from 'internal' namespace to a shared namespace.

[sffc, 2016-05-31 21:44:17]

Note, for completeness, I added two patches.  You can ignore patch 2.

https://codereview.appspot.com/300750043/diff/20001/common/locdspnm.cpp
File common/locdspnm.cpp (right):

https://codereview.appspot.com/300750043/diff/20001/common/locdspnm.cpp#newco...
common/locdspnm.cpp:272: namespace CapitalizationContextSinkNamespace {
On 2016/05/30 18:48:22, markus.icu wrote:
> Optional: In google3, this is often called "namespace internal".
> 
> Optional alternative: You could make CapitalizationContextSink a struct nested
> inside LocaleDisplayNamesImpl, more like in Java, and then you don't need the
> namespace and hopefully don't need to qualify things like
> LocaleDisplayNamesImpl::kCapContextUsageScript. You might still need the
friend
> declaration.

I really like the idea of a shared namespace!  It works great.  "friend" doesn't
appear to be needed either.  I'll declare all of my future data sink classes in
this way.

https://codereview.appspot.com/300750043/diff/20001/common/locdspnm.cpp#newco...
common/locdspnm.cpp:273: struct CapitalizationContextSink;
On 2016/05/30 18:48:22, markus.icu wrote:
> Please indent by 4 each level; check throughout the file.

Done, at least in the parts I edited.

https://codereview.appspot.com/300750043/diff/20001/common/locdspnm.cpp#newco...
common/locdspnm.cpp:411: #define CHECK_KEY_FOR_USAGE_ENUM(name, value) if
(uprv_strcmp(key, name) == 0) { usageEnum = value; }
On 2016/05/30 18:48:22, markus.icu wrote:
> Please don't use a macro for this, it makes for very hacky code.

The macro makes the code more concise and less repetitive (DRY) though.  It's
one of my favorite features of C/C++ versus Java  :) 

I removed the macro.

https://codereview.appspot.com/300750043/diff/20001/common/locdspnm.cpp#newco...
common/locdspnm.cpp:491: // Silently ignore.  Not every locale has
contextTransforms.
On 2016/05/30 18:48:22, markus.icu wrote:
> Don't you need to reset the status?

Yes.  Fixed.  Sort-of concerned that a test case didn't catch this.

[markus.icu, 2016-05-31 22:28:37]

LGTM

https://codereview.appspot.com/300750043/diff/80001/common/locdspnm.cpp
File common/locdspnm.cpp (right):

https://codereview.appspot.com/300750043/diff/80001/common/locdspnm.cpp#newco...
common/locdspnm.cpp:345: struct CapitalizationContextSink;
It looks strange to me to see a forward declaration of the nested class here but
the full declaration only later. Let's see if it works on all compilers... if it
does, I like it :-)

[sffc, 2016-05-31 22:45:54]

On 2016/05/31 22:28:37, markus.icu wrote:
> LGTM
> 
> https://codereview.appspot.com/300750043/diff/80001/common/locdspnm.cpp
> File common/locdspnm.cpp (right):
> 
>
https://codereview.appspot.com/300750043/diff/80001/common/locdspnm.cpp#newco...
> common/locdspnm.cpp:345: struct CapitalizationContextSink;
> It looks strange to me to see a forward declaration of the nested class here
but
> the full declaration only later. Let's see if it works on all compilers... if
it
> does, I like it :-)

Submitted as r38778.