Re: Changes in SingleFieldIdentity

2022-08-10 Thread Tilmann

Hi,

I think it's fine.

I think it can break code where people have a sorted list of
SingleFieldIdentity. This is probably rare (?).


An alternative might be to parametrize SingefieldIdentity:

public abstract class SingleFieldIdentity>
implements Externalizable,Comparable

and each subclass:

public class CharIdentity extends SingleFieldIdentity

I think that this way the SingleFieldIdentity stays comparable.

This seems to compile but I haven't tested it.

Tilmann


On 09/08/2022 23:48, Craig Russell wrote:

Hi,

Making this change seems like the right thing to do. While it changes the 
formal signature of the SingleFieldIdentity API class, no one will notice it 
unless (SignatureTest) they go  looking for it.

We just need to be careful. If there are concerns, we might consider putting 
this into 3.3 without going into a possible 3.2.2 release.

But I don't see anything amiss in the approach Michael suggests.

Regards
Craig


On Aug 9, 2022, at 06:28, Michael Bouschen  wrote:

Hi,

as part of JDO-817 "Check Compiler warnings" I was looking into the warnings of the api 
submodule. I started with "raw use of parameterized class", so replacing the use of Set by 
something like Set.

There is one case where I need some feedback. In javax.jdo.identity there is a 
class SingleFieldIdentity which is the abstract base class for single field 
identity classes such as IntIdentity, LongIdentity, etc. Today the abstract 
base class implements Comparable without implementing method compareTo which is 
done in the subclasses. The implements clause is using the raw type Comparable.

I would like to fix this and move the implements clause to the concrete 
subclasses, e.g.:

public class IntIdentity extends SingleFieldIdentity implements 
Comparable
public class LongIdentity extends SingleFieldIdentity implements 
Comparable
This way we can use the concrete subclass as class parameter for Comparable.

Then the signature of the compare method takes an instance of the concrete 
subclass as an argument instead of Object, such that we do not need the 
instanceof check anymore:

public int compareTo(LongIdentity o) {

instead of

public int compareTo(Object o) {
if (oinstanceof LongIdentity) {

The TCK succeeds with this change, but we would need to change the 
SignatureTest (runonce.conf), because of the change of the implements clause of 
SingleFieldIdentity and its subclasses.

What do you think? Is this an API change that is OK?

Regards Michael

Craig L Russell
c...@apache.org



Re: Changes in SingleFieldIdentity

2022-08-09 Thread Craig Russell
Hi,

Making this change seems like the right thing to do. While it changes the 
formal signature of the SingleFieldIdentity API class, no one will notice it 
unless (SignatureTest) they go  looking for it.

We just need to be careful. If there are concerns, we might consider putting 
this into 3.3 without going into a possible 3.2.2 release.

But I don't see anything amiss in the approach Michael suggests.

Regards
Craig

> On Aug 9, 2022, at 06:28, Michael Bouschen  wrote:
> 
> Hi,
> 
> as part of JDO-817 "Check Compiler warnings" I was looking into the warnings 
> of the api submodule. I started with "raw use of parameterized class", so 
> replacing the use of Set by something like Set.
> 
> There is one case where I need some feedback. In javax.jdo.identity there is 
> a class SingleFieldIdentity which is the abstract base class for single field 
> identity classes such as IntIdentity, LongIdentity, etc. Today the abstract 
> base class implements Comparable without implementing method compareTo which 
> is done in the subclasses. The implements clause is using the raw type 
> Comparable.
> 
> I would like to fix this and move the implements clause to the concrete 
> subclasses, e.g.:
> 
> public class IntIdentity extends SingleFieldIdentity implements 
> Comparable
> public class LongIdentity extends SingleFieldIdentity implements 
> Comparable 
> This way we can use the concrete subclass as class parameter for Comparable.
> 
> Then the signature of the compare method takes an instance of the concrete 
> subclass as an argument instead of Object, such that we do not need the 
> instanceof check anymore:
> 
> public int compareTo(LongIdentity o) {
> 
> instead of
> 
> public int compareTo(Object o) {
>if (oinstanceof LongIdentity) {
>
> The TCK succeeds with this change, but we would need to change the 
> SignatureTest (runonce.conf), because of the change of the implements clause 
> of SingleFieldIdentity and its subclasses.
> 
> What do you think? Is this an API change that is OK?
> 
> Regards Michael

Craig L Russell
c...@apache.org