Renaming numparse_unisets.h to static_unicode_sets.h and refactoring to guarantee safety in no-data…

ticket:13697 Renaming numparse_unisets.h to static_unicode_sets.h and refactoring to guarantee safety in no-data builds.

[sffc, 2018-05-15 03:11:37]

More file renaming and porting emptyness test to Java.

[markus.icu, 2018-05-15 21:32:50]

https://codereview.appspot.com/343170043/diff/20001/icu4c/source/common/stati...
File icu4c/source/common/static_unicode_sets.cpp (right):

https://codereview.appspot.com/343170043/diff/20001/icu4c/source/common/stati...
icu4c/source/common/static_unicode_sets.cpp:28: // Save the empty instance in
static memory to have well-defined behavior if a
Please add cleanup code. You can't deallocate this, but if it has been
initialized, you still need to call its constructor. Note that when we are out
of memory, this trick gets you around having a nullptr for the set, but it will
then not be able to allocate its internals.

Also, you should ensure that this is aligned on a proper boundary for the
machine. Please look for something like UAlignedMemory and how it has been used.

https://codereview.appspot.com/343170043/diff/20001/icu4c/source/common/stati...
icu4c/source/common/static_unicode_sets.cpp:144: // NOTE: It is OK for these
assertions to fail if there was a no-data build.
Assertions are useful for debugging, but they are omitted in a non-debug build.
Do not assume that the sets are non-nullptr, you have to check for it
explicitly.

https://codereview.appspot.com/343170043/diff/20001/icu4c/source/common/stati...
File icu4c/source/common/static_unicode_sets.h (right):

https://codereview.appspot.com/343170043/diff/20001/icu4c/source/common/stati...
icu4c/source/common/static_unicode_sets.h:60: // Also used to indicate null in
chooseFrom().
I suggest you define a NONE = -1 and make call sites test < 0.

https://codereview.appspot.com/343170043/diff/20001/icu4j/main/classes/core/s...
File icu4j/main/classes/core/src/com/ibm/icu/impl/StaticUnicodeSets.java
(right):

https://codereview.appspot.com/343170043/diff/20001/icu4j/main/classes/core/s...
icu4j/main/classes/core/src/com/ibm/icu/impl/StaticUnicodeSets.java:65: private
static final Map<Key, UnicodeSet> unicodeSets = new EnumMap<Key,
UnicodeSet>(Key.class);
Do we actually need the Map in Java?
In Java, it is customary to have static final constants that are initialized at
class-load-time, such as StaticUnicodeSets.DEFAULT_IGNORABLES.

https://codereview.appspot.com/343170043/diff/20001/icu4j/main/classes/core/s...
icu4j/main/classes/core/src/com/ibm/icu/impl/StaticUnicodeSets.java:226: //
NOTE: It is OK for these assertions to fail if there was a no-data build.
Same as in C++, have to check with more than just "assert".

[markus.icu, 2018-05-15 21:34:56]

Looking at the ticket comment now. I commented on the assertions assuming that
you still relied on them, but if you are not actually relying on just assertions
anywhere any more, then you can ignore those two comments.

[sffc, 2018-05-15 21:53:30]

Replying to Markus feedback.

[sffc, 2018-05-15 21:54:23]

Fixing APIdocs.

[sffc, 2018-05-15 21:56:00]

https://codereview.appspot.com/343170043/diff/20001/icu4c/source/common/stati...
File icu4c/source/common/static_unicode_sets.cpp (right):

https://codereview.appspot.com/343170043/diff/20001/icu4c/source/common/stati...
icu4c/source/common/static_unicode_sets.cpp:28: // Save the empty instance in
static memory to have well-defined behavior if a
On 2018/05/15 21:32:44, markus.icu wrote:
> Please add cleanup code. You can't deallocate this, but if it has been
> initialized, you still need to call its constructor. Note that when we are out
> of memory, this trick gets you around having a nullptr for the set, but it
will
> then not be able to allocate its internals.
> 
> Also, you should ensure that this is aligned on a proper boundary for the
> machine. Please look for something like UAlignedMemory and how it has been
used.

Done.  I'm somewhat surprised though that an empty UnicodeSet needs a destructor
call?  I was assuming that an empty UnicodeSet is clean to release without a
destructor (i.e., no dangling references).

https://codereview.appspot.com/343170043/diff/20001/icu4c/source/common/stati...
icu4c/source/common/static_unicode_sets.cpp:144: // NOTE: It is OK for these
assertions to fail if there was a no-data build.
On 2018/05/15 21:32:43, markus.icu wrote:
> Assertions are useful for debugging, but they are omitted in a non-debug
build.
> Do not assume that the sets are non-nullptr, you have to check for it
> explicitly.

Acknowledged.  They are never read without a nullptr check.

https://codereview.appspot.com/343170043/diff/20001/icu4c/source/common/stati...
File icu4c/source/common/static_unicode_sets.h (right):

https://codereview.appspot.com/343170043/diff/20001/icu4c/source/common/stati...
icu4c/source/common/static_unicode_sets.h:60: // Also used to indicate null in
chooseFrom().
On 2018/05/15 21:32:45, markus.icu wrote:
> I suggest you define a NONE = -1 and make call sites test < 0.

Done.

https://codereview.appspot.com/343170043/diff/20001/icu4j/main/classes/core/s...
File icu4j/main/classes/core/src/com/ibm/icu/impl/StaticUnicodeSets.java
(right):

https://codereview.appspot.com/343170043/diff/20001/icu4j/main/classes/core/s...
icu4j/main/classes/core/src/com/ibm/icu/impl/StaticUnicodeSets.java:65: private
static final Map<Key, UnicodeSet> unicodeSets = new EnumMap<Key,
UnicodeSet>(Key.class);
On 2018/05/15 21:32:46, markus.icu wrote:
> Do we actually need the Map in Java?
> In Java, it is customary to have static final constants that are initialized
at
> class-load-time, such as StaticUnicodeSets.DEFAULT_IGNORABLES.

But if you do that then you can't work with the enums.  There's logic elsewhere
that first figures out the right enum to use and then gets the UnicodeSet once
it's figured out the right enum.

https://codereview.appspot.com/343170043/diff/20001/icu4j/main/classes/core/s...
icu4j/main/classes/core/src/com/ibm/icu/impl/StaticUnicodeSets.java:226: //
NOTE: It is OK for these assertions to fail if there was a no-data build.
On 2018/05/15 21:32:47, markus.icu wrote:
> Same as in C++, have to check with more than just "assert".

The get() method always checks for null before returning a UnicodeSet.  All
accesses go through that method.

[sffc, 2018-05-15 22:05:17]

https://codereview.appspot.com/343170043/diff/20001/icu4c/source/common/stati...
File icu4c/source/common/static_unicode_sets.cpp (right):

https://codereview.appspot.com/343170043/diff/20001/icu4c/source/common/stati...
icu4c/source/common/static_unicode_sets.cpp:28: // Save the empty instance in
static memory to have well-defined behavior if a
On 2018/05/15 21:55:59, sffc wrote:
> On 2018/05/15 21:32:44, markus.icu wrote:
> > Please add cleanup code. You can't deallocate this, but if it has been
> > initialized, you still need to call its constructor. Note that when we are
out
> > of memory, this trick gets you around having a nullptr for the set, but it
> will
> > then not be able to allocate its internals.
> > 
> > Also, you should ensure that this is aligned on a proper boundary for the
> > machine. Please look for something like UAlignedMemory and how it has been
> used.
> 
> Done.  I'm somewhat surprised though that an empty UnicodeSet needs a
destructor
> call?  I was assuming that an empty UnicodeSet is clean to release without a
> destructor (i.e., no dangling references).

I missed the comment about UAlignedMemory.  Do I just add a line that says
"UAlignedMemory deadSpace;" above the gEmptyUnicodeSet definition?

[markus.icu, 2018-05-16 02:25:35]

On 2018/05/15 22:05:17, sffc wrote:
> I missed the comment about UAlignedMemory.  Do I just add a line that says
> "UAlignedMemory deadSpace;" above the gEmptyUnicodeSet definition?

No. Please use something like
UAlignedMemory gEmptyUnicodeSet[sizeof(UnicodeSet) / sizeof(UAlignedMemory) +
1];
to ensure that the gEmptyUnicodeSet itself is aligned.

I see a couple of existing places with this "deadSpace"; I think they are
unreliable. Ticket please?

[markus.icu, 2018-05-16 02:36:34]

https://codereview.appspot.com/343170043/diff/20001/icu4c/source/common/stati...
File icu4c/source/common/static_unicode_sets.cpp (right):

https://codereview.appspot.com/343170043/diff/20001/icu4c/source/common/stati...
icu4c/source/common/static_unicode_sets.cpp:28: // Save the empty instance in
static memory to have well-defined behavior if a
On 2018/05/15 21:55:59, sffc wrote:
> Done.  I'm somewhat surprised though that an empty UnicodeSet needs a
destructor
> call?  I was assuming that an empty UnicodeSet is clean to release without a
> destructor (i.e., no dangling references).

An empty UnicodeSet may or may not heap-allocate stuff. Making assumptions is
setting up future maintainers for memory leaks.

https://codereview.appspot.com/343170043/diff/60001/icu4c/source/common/stati...
File icu4c/source/common/static_unicode_sets.cpp (right):

https://codereview.appspot.com/343170043/diff/60001/icu4c/source/common/stati...
icu4c/source/common/static_unicode_sets.cpp:122: gEmptyUnicodeSetInitialized =
false;
FALSE

https://codereview.appspot.com/343170043/diff/60001/icu4c/source/common/stati...
icu4c/source/common/static_unicode_sets.cpp:138: gEmptyUnicodeSetInitialized =
true;
TRUE

https://codereview.appspot.com/343170043/diff/60001/icu4c/source/i18n/numpars...
File icu4c/source/i18n/numparse_decimal.cpp (right):

https://codereview.appspot.com/343170043/diff/60001/icu4c/source/i18n/numpars...
icu4c/source/i18n/numparse_decimal.cpp:44: if (decimalKey != unisets::NONE) {
if (decimalKey >= 0) {

https://codereview.appspot.com/343170043/diff/60001/icu4c/source/i18n/numpars...
icu4c/source/i18n/numparse_decimal.cpp:54: if (groupingKey != unisets::NONE &&
decimalKey != unisets::NONE) {
if (... >= 0  2x

[sffc, 2018-05-16 22:05:45]

https://codereview.appspot.com/343170043/diff/60001/icu4c/source/common/stati...
File icu4c/source/common/static_unicode_sets.cpp (right):

https://codereview.appspot.com/343170043/diff/60001/icu4c/source/common/stati...
icu4c/source/common/static_unicode_sets.cpp:122: gEmptyUnicodeSetInitialized =
false;
On 2018/05/16 02:36:34, markus.icu wrote:
> FALSE

Done.

https://codereview.appspot.com/343170043/diff/60001/icu4c/source/common/stati...
icu4c/source/common/static_unicode_sets.cpp:138: gEmptyUnicodeSetInitialized =
true;
On 2018/05/16 02:36:34, markus.icu wrote:
> TRUE

Done.

https://codereview.appspot.com/343170043/diff/60001/icu4c/source/i18n/numpars...
File icu4c/source/i18n/numparse_decimal.cpp (right):

https://codereview.appspot.com/343170043/diff/60001/icu4c/source/i18n/numpars...
icu4c/source/i18n/numparse_decimal.cpp:44: if (decimalKey != unisets::NONE) {
On 2018/05/16 02:36:34, markus.icu wrote:
> if (decimalKey >= 0) {

Done.

https://codereview.appspot.com/343170043/diff/60001/icu4c/source/i18n/numpars...
icu4c/source/i18n/numparse_decimal.cpp:54: if (groupingKey != unisets::NONE &&
decimalKey != unisets::NONE) {
On 2018/05/16 02:36:34, markus.icu wrote:
> if (... >= 0  2x

Done.

[sffc, 2018-05-16 22:05:45]

Replying to Markus feedback round2

[markus.icu, 2018-05-16 22:26:51]

LGTM