RFR[9] 8160956: Runtime.Version.compareTo/compareToIgnoreOpt problem

classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

RFR[9] 8160956: Runtime.Version.compareTo/compareToIgnoreOpt problem

Pavel Rappo
Hello,

Please review the following trivial change for [1]:

  http://cr.openjdk.java.net/~prappo/8160956/webrev.00/

The code change fixes the implementation and makes it adhere to the
specification for Version comparison:

    /**
     * Compares this version to another.
     *
        ...
     * <p> A version without a build number is always less than one with a
     * build number; otherwise build numbers are compared numerically. </p>
        ...
     */
    @Override
    public int compareTo(Version ob)

The existing test was incorrectly testing these cases. So rather than bringing
the test case provided in the linked issue, I updating those existing cases.

The patch also fixes a bunch of javadoc typos.

P.S. While we are in this area, may I ask if anybody knows why exactly Version
class has lost its 'final' modifier while moving from 'jdk' package to
'java.lang' [2]?

Thanks,
-Pavel

--------------------------------------------------------------------------------
[1] https://bugs.openjdk.java.net/browse/JDK-8160956
[2] https://bugs.openjdk.java.net/browse/JDK-8144062

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR[9] 8160956: Runtime.Version.compareTo/compareToIgnoreOpt problem

mark.reinhold
2017/3/16 8:17:03 -0700, [hidden email]:
> Please review the following trivial change for [1]:
>
>   http://cr.openjdk.java.net/~prappo/8160956/webrev.00/

Looks good to me.

> ...
>
> P.S. While we are in this area, may I ask if anybody knows why exactly Version
> class has lost its 'final' modifier while moving from 'jdk' package to
> 'java.lang' [2]?

I have no idea.  This class really ought to be final.

- Mark
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR[9] 8160956: Runtime.Version.compareTo/compareToIgnoreOpt problem

Paul Sandoz

> On 16 Mar 2017, at 08:27, [hidden email] wrote:
>
> 2017/3/16 8:17:03 -0700, [hidden email]:
>> Please review the following trivial change for [1]:
>>
>>  http://cr.openjdk.java.net/~prappo/8160956/webrev.00/
>
> Looks good to me.
>
>> ...
>>
>> P.S. While we are in this area, may I ask if anybody knows why exactly Version
>> class has lost its 'final' modifier while moving from 'jdk' package to
>> 'java.lang' [2]?
>
> I have no idea.  This class really ought to be final.
>

Yes, Pavel unless there is some non-obvious here (which i think is unlikely) i suggest you make it so, update your patch, test, then push!

Paul.
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR[9] 8160956: Runtime.Version.compareTo/compareToIgnoreOpt problem

Pavel Rappo
Paul,

One more question. I have read Version's javadoc and my impression is that
Version could be a value-type class. Is that right?

> On 16 Mar 2017, at 18:05, Paul Sandoz <[hidden email]> wrote:
>
>>
>> On 16 Mar 2017, at 08:27, [hidden email] wrote:
>>
>> 2017/3/16 8:17:03 -0700, [hidden email]:
>>> Please review the following trivial change for [1]:
>>>
>>> http://cr.openjdk.java.net/~prappo/8160956/webrev.00/
>>
>> Looks good to me.
>>
>>> ...
>>>
>>> P.S. While we are in this area, may I ask if anybody knows why exactly Version
>>> class has lost its 'final' modifier while moving from 'jdk' package to
>>> 'java.lang' [2]?
>>
>> I have no idea.  This class really ought to be final.
>>
>
> Yes, Pavel unless there is some non-obvious here (which i think is unlikely) i suggest you make it so, update your patch, test, then push!
>
> Paul.

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR[9] 8160956: Runtime.Version.compareTo/compareToIgnoreOpt problem

Paul Sandoz

> On 16 Mar 2017, at 13:27, Pavel Rappo <[hidden email]> wrote:
>
> Paul,
>
> One more question. I have read Version's javadoc and my impression is that
> Version could be a value-type class. Is that right?
>

I suppose it could if there were such a thing as of today.

It's not explicitly called out as being value-based as is the case for Optional, is that where your are thinking of heading?

Paul.

>> On 16 Mar 2017, at 18:05, Paul Sandoz <[hidden email]> wrote:
>>
>>>
>>> On 16 Mar 2017, at 08:27, [hidden email] wrote:
>>>
>>> 2017/3/16 8:17:03 -0700, [hidden email]:
>>>> Please review the following trivial change for [1]:
>>>>
>>>> http://cr.openjdk.java.net/~prappo/8160956/webrev.00/
>>>
>>> Looks good to me.
>>>
>>>> ...
>>>>
>>>> P.S. While we are in this area, may I ask if anybody knows why exactly Version
>>>> class has lost its 'final' modifier while moving from 'jdk' package to
>>>> 'java.lang' [2]?
>>>
>>> I have no idea.  This class really ought to be final.
>>>
>>
>> Yes, Pavel unless there is some non-obvious here (which i think is unlikely) i suggest you make it so, update your patch, test, then push!
>>
>> Paul.
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR[9] 8160956: Runtime.Version.compareTo/compareToIgnoreOpt problem

Pavel Rappo
Exactly:

*
* <p>This is a <a href="../lang/doc-files/ValueBased.html">value-based</a>
* class; use of identity-sensitive operations (including reference equality
* ({@code ==}), identity hash code, or synchronization) on instances of
* {@code Optional} may have unpredictable results and should be avoided.
*


> On 16 Mar 2017, at 20:41, Paul Sandoz <[hidden email]> wrote:
>
> is that where your are thinking of heading?

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR[9] 8160956: Runtime.Version.compareTo/compareToIgnoreOpt problem

mark.reinhold
2017/3/16 14:31:21 -0700, [hidden email]:
> Exactly:
>
> *
> * <p>This is a <a href="../lang/doc-files/ValueBased.html">value-based</a>
> * class; use of identity-sensitive operations (including reference equality
> * ({@code ==}), identity hash code, or synchronization) on instances of
> * {@code Optional} may have unpredictable results and should be avoided.
> *

An excellent plan.

- Mark
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR[9] 8160956: Runtime.Version.compareTo/compareToIgnoreOpt problem

Paul Sandoz

> On 16 Mar 2017, at 14:33, [hidden email] wrote:
>
> 2017/3/16 14:31:21 -0700, [hidden email]:
>> Exactly:
>>
>> *
>> * <p>This is a <a href="../lang/doc-files/ValueBased.html">value-based</a>
>> * class; use of identity-sensitive operations (including reference equality
>> * ({@code ==}), identity hash code, or synchronization) on instances of
>> * {@code Optional} may have unpredictable results and should be avoided.
>> *
>
> An excellent plan.
>

Yes +1

Paul.
Loading...