[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-core-libs-dev
Subject:    Re: RFR(xs): 8059361: Properties.stringPropertyNames() returns a set inconsistent with the assertion
From:       Mandy Chung <mandy.chung () oracle ! com>
Date:       2016-05-26 22:56:41
Message-ID: 5FE0EF4F-EEE9-4601-8BF2-DE9019DA5CED () oracle ! com
[Download RAW message or body]

On May 26, 2016, at 3:49 PM, Stuart Marks <stuart.marks@oracle.com> wrote:
> 
> 
> 
> On 5/26/16 2:28 PM, Mandy Chung wrote:
> > > -        return h.keySet();
> > > +        return Set.of(h.keySet().toArray(new String[0]));
> > > }
> > 
> > The patch looks fine.  It'd be good to add a test case.
> > 
> > If you use Collections.unmodifiableSet, you would not need to convert the key \
> > sets to an array. Any benefit of using Set::of instead of \
> > Collections.unmodifiableSet?
> 
> There are several minor tradeoffs.
> 
> Collections.unmodifiableSet() simply creates and returns a wrapper object. It \
> contains the keySet, which is backed by the HashMap created within the \
> stringPropertyNames() method. This keeps references to all the string keys and \
> values around, even though the values aren't used. But the strings themselves are \
> also referenced from the original Properties object and its default chain. 
> The toArray() call copies the key string refs to an array, then Set.of() hashes \
> them into its internal array. But the resulting data structure holds only \
> references to the keys, so it's more compact. 
> Given that most uses seem to iterate the set's elements and then throw it away, \
> it's probably not worth the creation overhead of for an immutable Set. Drat. I \
> guess I was too eager to use the new API. :-) 
> I've changed this to use Collections.unmodifiableSet(); see below.
> 

Looks fine.  

I don't have any objection in using Set::of vs Collections.unmodifiableSet() in this \
case (Sorry.  I should have made it clear).  No need for new patch if you decide to \
use Set.of (a comment might help).

Mandy=


[prev in list] [next in list] [prev in thread] [next in thread]