[loc-en-dev] Code review request for 6875847: Java Locale Enhancement
y.umaoka at gmail.com
Mon Aug 30 12:07:14 PDT 2010
Thanks for reviewing the code.
> Here are my comments for the changes:
> - 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.
> - 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
I think it's not a good idea because String.toLowerCase() depends on a
- It introduces cyclic dependency
- Locale sensitivity is harmful
- AsciiUtil.toLowerString() is the lightest possible implementation for
> - 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.
> - no need to import ResourceBundle.Control
Right. I'll remove the import.
> - Should implement a proper hashCode() and cache the value, as in
OK. It is not necessary, but if that is the convention, I'll do so.
> - No need to define LOCALESEP, as it is a duplicate of BaseLocale.SEP
OK. I'll update.
> - Do we really need this? Can InternalLocaleBuilder just throw
Actually, it is not necessary. But it makes us much easier to share the
implementation between JDK and ICU.
Is this a problem?
> - Need GPL2 copyright header
Sorry, I missed it. I'll update.
> (not in the list)
> - should contain new files.
OK, I'll update.
More information about the locale-enhancement-dev