RFR: 6543126: Level.known can leak memory

[Peter Levart]

Hi Daniel,

Now that you have various findByXXX methods return Optional<Level>, you 
could make methods that use them more Optional-API-powered. For example, 
findLevel could be written as a single expression:

     static Level findLevel(String name) {
         return KnownLevel
             .findByName(Objects.requireNonNull(name), KnownLevel::mirrored)
             .or(() -> {
                 // Now, check if the given name is an integer.  If so,
                 // first look for a Level with the given value and then
                 // if necessary create one.
                 try {
                     int x = Integer.parseInt(name);
                     return KnownLevel
                         .findByValue(x, KnownLevel::mirrored)
                         .or(() -> {
                             // add new Level
                             new Level(name, x);
                             return KnownLevel.findByValue(x, 
KnownLevel::mirrored);
                         });
                 } catch (NumberFormatException ex) {
                     // Not an integer.
                     // Drop through.
                     return Optional.empty();
                 }
             })
             .or(() -> KnownLevel.findByLocalizedLevelName(name, 
KnownLevel::mirrored))
             .orElse(null);
     }


Same with parse:

     public static synchronized Level parse(String name) throws 
IllegalArgumentException {
         return KnownLevel
             // Look for a known Level with the given non-localized name.
             .findByName(Objects.requireNonNull(name), KnownLevel::referent)
             .or(() -> {
                 // Now, check if the given name is an integer.  If so,
                 // first look for a Level with the given value and then
                 // if necessary create one.
                 try {
                     int x = Integer.parseInt(name);
                     return KnownLevel
                         .findByValue(x, KnownLevel::referent)
                         .or(() -> {
                             // add new Level.
                             new Level(name, x);
                             return KnownLevel.findByValue(x, 
KnownLevel::referent);
                         });
                 } catch (NumberFormatException ex) {
                     // Not an integer.
                     // Drop through.
                     return Optional.empty();
                 }
             })
             // Finally, look for a known level with the given localized 
name,
             // in the current default locale.
             // This is relatively expensive, but not excessively so.
             .or(() -> KnownLevel.findByLocalizedLevelName(name, 
KnownLevel::referent))
             // OK, we've tried everything and failed
             .orElseThrow(() -> new IllegalArgumentException("Bad level 
\"" + name + "\""));
     }


Regards, Peter


On 08/16/2016 12:42 PM, Daniel Fuchs wrote:
> Hi Mandy,
>
> I added an additional selector parameter to the find methods.
> This made it possible to return Optional<Level> instead of
> KnownLevel - and it does simply the parse() method.
>
> http://cr.openjdk.java.net/~dfuchs/webrev_6543126/webrev.02
>
> best regards,
>
> -- daniel
>
> On 11/08/16 20:12, Mandy Chung wrote:
>>
>>> On Aug 11, 2016, at 2:29 AM, Daniel Fuchs <daniel.fuchs at oracle.com> 
>>> wrote:
>>>
>>> On 10/08/16 17:21, Mandy Chung wrote:
>>>>> On Jul 29, 2016, at 4:54 AM, Daniel Fuchs 
>>>>> <daniel.fuchs at oracle.com> wrote:
>>>>>>
>>>>>> http://cr.openjdk.java.net/~dfuchs/webrev_6543126/webrev.01/
>>>> This looks pretty good.
>>>>
>>>> Since KnownLevel is now a Reference, I suggest to change 
>>>> KnownLevel::findByName, findByValue and findByLocalizedLevelName to 
>>>> return Optional<Level> instead such that the parse method 
>>>> implementaiton could be simplified.
>>>
>>> We need to return KnownLevel because sometimes we need the
>>> level object and sometimes the mirror.
>>
>> So either findByName(String name, boolean mirror) or two methods: 
>> findLevelByName and findMirroredLevelByName??
>>
>> Or seriously consider to remove KnownLevel class by introducing a new 
>> Level subclass with final Level.getName, Level.getLocalizedName, 
>> Level.getResourceBundleName methods??
>>
>> Mandy
>>
>