RFR 8066291: ZoneIdPrinterParser can be optimized

[Roger Riggs]

Hi Stephen,

The aspect of the current implementation that is problematic is the 
copying of the set,
its not just single object creation but an entry for every ZoneID.

Adding a size method exposing some internal state increases the 
possibility that
when it is used independently it will be out of sync.  Not a big issue, 
but one to avoid
when designing an API.

Exposing a mutable Hashset was probably a mistake but one that cannot be 
corrected
now.  The optics aren't concerning to me.

SharedSecrets are much messier and not appropriate.

Adding a method to provide what was originally needed is a cleaner solution.

Roger



On 5/5/2016 11:27 AM, Stephen Colebourne wrote:
> I fail to see why adding a new read-only method alongside the existing
> method adds any more value to the API than adding a new size method.
> At least with the size method the API is still sensible - a mutable
> and immutable method alongside each other shouts out that a mistake
> was made. A size method is more subtle about the mistake (plenty of
> APIs have a size method alongside a collection method).
>
> Finally, a read-only method involves object creation, thus has worse
> performance than adding a size method.
>
> The only other alternative is perhaps to use SharedSecrets? I don't
> know what possibilities there are there. If not SharedSecrets, then no
> matter what, we are adding "a trivial method to the public API used
> only for an optimization".
>
> Stephen
>
>
> On 5 May 2016 at 15:23, Roger Riggs <Roger.Riggs at oracle.com> wrote:
>> Hi Bhanu,
>>
>> Adding a trivial method to the public API used only for an optimization is
>> not a good fix for this issue.
>>
>> A better fix was suggested to add a non-copying read-only version of
>>
>> ZoneRulesProvider.getAvailableZoneIds()
>>
>> Please revise the fix to instead implement and use:
>>
>>    /**
>>       * Gets a readonly set of available zone IDs.
>>       * <p>
>>       * These IDs are the string form of a {@link ZoneId}.
>>       *
>>       * @return a unmodifiable copy of the set of zone IDs, not null
>>       */
>>      public static Set<String> getReadOnlyAvailableZoneIds() {
>>          return Collections.unmodifiableSet(ZONES.keySet());
>>      }
>>
>> Roger
>>
>>
>>
>> On 5/5/2016 5:10 AM, Bhanu Gopularam wrote:
>>
>> Hi all,
>>
>>
>>
>> Please review fix following issue
>>
>>
>>
>> Bug Id : https://bugs.openjdk.java.net/browse/JDK-8066291
>>
>>
>>
>> Solution: provided new method to get size of available zone ids
>>
>>
>>
>> webrev : http://cr.openjdk.java.net/~bgopularam/bhanu/JDK-8066291/webrev.00
>>
>>
>>
>> Thanks,
>>
>> Bhanu
>>
>>