| [DataBinding] IdentityWrapper violates requirements for equals [message #447927] |
Tue, 18 April 2006 03:19  |
Eclipse User |
|
|
|
The current implementation of IdentityWrapper violates the basic
requirements for equals (see
http://java.sun.com/j2se/1.4.2/docs/api/java/lang/Object.htm l#equals(java.lang.Object)),
namely
"For any non-null reference value x, x.equals(null) should return false"
because it can't handle null values. The current implementation:
public boolean equals(Object obj) {
if (obj.getClass() != IdentityWrapper.class) {
return false;
}
return o == ((IdentityWrapper) obj).o; }
should be changed to:
public boolean equals(Object obj) {
if (obj == null || obj.getClass() != IdentityWrapper.class) {
return false;
}
return o == ((IdentityWrapper) obj).o; }
to fix this issue. (Note: I'm aware that IdentityWrapper is an internal
class, but it looks like a class which might be made public in the
future, so I mentioned this point)
Greetings from Bremen,
Daniel Krügler
|
|
|
|
|
|
| Re: [DataBinding] IdentityWrapper violates requirements for equals [message #448271 is a reply to message #448227] |
Thu, 20 April 2006 02:37  |
| Eclipse User |
|
|
|
Brad Reynolds wrote:
> I was actually wanting to try to look at all of these and understand
> what you're needing but haven't had the time yet. Also 3.2 is winding
> down so Boris and David are both probably really busy and might not get
> to the posts anytime soon. Hopefully I didn't come off as sounding
> ungrateful or that I thought posting them here was wrong, just trying to
> ensure that they get addressed.
I absolutely understand, that time is critical now and your tone was
absolutely OK for me. As suggested in the bugzilla report, I attached a
proposed patch. I hope I did it as you usually do (I tried the "Create
patch" command the first time today and recognized some options
concerning the output format)
> What do you mean when you say "which actually aren't any because of
> reasons hidden to me?"
From a local perspective the equals implementation of IdentityWrapper is
buggy because there exists legal conditions when an NPE could arise.
But considering that this class is at least currently strictly internal,
one could argue, that the current internal use excludes situations where
the equals arguments could be null at all. Since I didn't proofed such
a critical situation in the currently existing internal source code
based on this class, I sayed "because of reasons hidden to me".
Btw.: There exists another internal inner (static) class, namely
ListenerSupport.IdentityWrapper
which is essentially a copy of the current global IdentityWrapper class,
that is, it has the same defect concerning its equals impl. May I
suggest to replace this complete class by the global (fixed)
IdentityWrapper class? I could also provide a patch, if you like.
Greetings from Bremen,
Daniel Krügler
|
|
|
| Re: [DataBinding] IdentityWrapper violates requirements for equals [message #448281 is a reply to message #448271] |
Thu, 20 April 2006 09:16 |
| Eclipse User |
|
|
|
On 2006-04-20 00:37:48 -0600, Daniel Krügler <dsp@bdal.de> said:
> Brad Reynolds wrote:
>> I was actually wanting to try to look at all of these and understand
>> what you're needing but haven't had the time yet. Also 3.2 is winding
>> down so Boris and David are both probably really busy and might not get
>> to the posts anytime soon. Hopefully I didn't come off as sounding
>> ungrateful or that I thought posting them here was wrong, just trying
>> to ensure that they get addressed.
>
> I absolutely understand, that time is critical now and your tone was
> absolutely OK for me. As suggested in the bugzilla report, I attached a
> proposed patch. I hope I did it as you usually do (I tried the "Create
> patch" command the first time today and recognized some options
> concerning the output format)
It looks good. That's the way to do it, or at least that's how I do it.
>
>> What do you mean when you say "which actually aren't any because of
>> reasons hidden to me?"
>
> From a local perspective the equals implementation of IdentityWrapper is
> buggy because there exists legal conditions when an NPE could arise.
> But considering that this class is at least currently strictly internal,
> one could argue, that the current internal use excludes situations where
> the equals arguments could be null at all. Since I didn't proofed such
> a critical situation in the currently existing internal source code
> based on this class, I sayed "because of reasons hidden to me".
Thanks for the explanation, I understand now.
>
> Btw.: There exists another internal inner (static) class, namely
>
> ListenerSupport.IdentityWrapper
>
> which is essentially a copy of the current global IdentityWrapper class,
> that is, it has the same defect concerning its equals impl. May I
> suggest to replace this complete class by the global (fixed)
> IdentityWrapper class? I could also provide a patch, if you like.
If you have the time please provide the patch and a bug report.
They're always appreciated.
>
> Greetings from Bremen,
>
> Daniel Krügler
|
|
|
| Re: [DataBinding] IdentityWrapper violates requirements for equals [message #448287 is a reply to message #448281] |
Thu, 20 April 2006 10:44 |
| Eclipse User |
|
|
|
Brad Reynolds wrote:
>> Btw.: There exists another internal inner (static) class, namely
>>
>> ListenerSupport.IdentityWrapper
>>
>> which is essentially a copy of the current global IdentityWrapper class,
>> that is, it has the same defect concerning its equals impl. May I
>> suggest to replace this complete class by the global (fixed)
>> IdentityWrapper class? I could also provide a patch, if you like.
>
>
> If you have the time please provide the patch and a bug report. They're
> always appreciated.
Bug report and proposed patch have been provided here:
https://bugs.eclipse.org/bugs/show_bug.cgi?id=137740
Greetings from Bremen,
Daniel Krügler
|
|
|
Powered by
FUDForum. Page generated in 0.03720 seconds
Home