[loc-en-dev] Code review request for 6875847: Java Locale Enhancement

[Yoshito Umaoka]

Thanks for reviewing the code.

> Here are my comments for the changes:
>
> General:
> - Instead of String.equals(), use of "==" for comparing locale 
> elements (language/script/country/variant), as they are all intern'ed.
I think you're talking about BaseLocale.equals.

It actually does matter whether we use String#equals() or ==, because it 
does not create multiple BaseLocale instances with same 
language/script/country/variant. So technically, BaseLocale.equals() can 
be simply - return (this == obj).

I wanted to make the implementation more robust - even we change the 
implementation, BaseLocale.equals() continue to work properly. But if 
you feel this strongly, it's OK to update the code to use "==" (although 
it does not have any performance improvement with it).

For LocaleExtensions.equals - it is not intern'd - and we probably do 
not need to do so, because non-empty extensions are not commonly used.

> - Remove JDKIMPL check. Especially the code block for !JDKIMPL that 
> won't be executed.
>
OK - will do.

> Locale.java
> - private Locale(lang,scrpt,ctry,vrnt) constructor is redundant. Its 
> implementation can just be moved to Locale(lang,ctry,vrnt) with script 
> = "".
OK - will do.

> - Do we need two caches, one in BaseLocale and the other in Locale 
> which in most cases a duplicate (excl. EMPTY_EXTENSIONS)? Can we get 
> rid of the redundancy?
The idea was

- When Locale is created by factory methods (Builder / forLanguageTag), 
we do not want to create multiple Locale instances representing the same 
locale. Thus, we have Locale cache
- However, we cannot get rid of Locales created by the constructors. For 
them - we just try to avoid multiple BaseLocale instances representing 
the same locale are created. Thus, we also do BaseLocale level caching.

The overhead is - one extra map structure. But we do not create 
redundant BaseLocale objects, so it should not be so bad.

> - toLowerCase() can be removed with the replacement of 
> AsciiUtils.toLowerString()
I think it's not a good idea because String.toLowerCase() depends on a 
locale.

- It introduces cyclic dependency
- Locale sensitivity is harmful
- AsciiUtil.toLowerString() is the lightest possible implementation for 
this.

> - hashCode() comment looks wrong with the change.
Can you tell me which comment you're talking about?

BTW, I realized that Locale.hashCode should probably use XOR, not OR. 
I'll update this.

>
> ResourceBundle.java
> - no need to import ResourceBundle.Control
Right. I'll remove the import.
>
> Extension.java
> - Should implement a proper hashCode() and cache the value, as in 
> BaseLocale
OK. It is not necessary, but if that is the convention, I'll do so.

>
> InternalLocaleBuilder.java
> - No need to define LOCALESEP, as it is a duplicate of BaseLocale.SEP
>
OK. I'll update.
> LocaleSyntaxException.java
> - Do we really need this? Can InternalLocaleBuilder just throw 
> IllformedLocaleException?
>
Actually, it is not necessary. But it makes us much easier to share the 
implementation between JDK and ICU.
Is this a problem?

> StringTokenIterator.java
> - Need GPL2 copyright header
>
Sorry, I missed it. I'll update.
> (not in the list)
> make/java/java/FILES_java.gmk
> - should contain new files.
>
OK, I'll update.