[#HHH-1646] Bad code in FastClass.equals

Hibernate Core

Bad code in FastClass.equals

Created: 06/Apr/06 05:32 PM Updated: 15/Dec/06 10:32 AM

Component/s:

None

Affects Version/s:

3.1.3, 3.2.0 cr1

Fix Version/s:

3.2.2

Time Tracking:

Original Estimate:

30 minutes

Remaining Estimate:

30 minutes

Time Spent:

Not Specified

File Attachments:	1. FastClass.patch (0.5 kB)
Environment:	3.2.0

Description

« Hide

In FastClass, the equals method is:
  public boolean equals(Object o) {
    if ( o == null || ( o instanceof FastClass ) ) {
      return false;
    }
    return this.type.equals( ( ( FastClass ) o ).type );
}

There's a missing ! - the first line should be

if ( o == null || ! ( o instanceof FastClass ) ) {

All

Comments

Work Log

Change History

FishEye

Sort Order:

[ Permalink | « Hide ]

Max Rydahl Andersen added a comment - 07/Apr/06 01:55 AM

higher priority - need to apply to proper 3.2 branch too.

[ Permalink | « Hide ]

Konstantin Ignatyev added a comment - 19/May/06 11:43 AM

Patch that implements that

[ Permalink | « Hide ]

Greg Matthews added a comment - 21/May/06 07:44 PM

You don't strictly really need the null check either. The instanceof operator handles null. i.e. the following will work just fine.

if(o instanceof FastClass) {
return this.type.equals(((FastClass)o).type);
}
return false; // if o is null, we'll return false

[ Permalink | « Hide ]

Max Rydahl Andersen added a comment - 20/Jul/06 03:51 AM

greg: no, null instanceof X is always true.

[ Permalink | « Hide ]

Max Rydahl Andersen added a comment - 20/Jul/06 03:55 AM

scratch that - i negated one time to many ;)

[ Permalink | « Hide ]

Scott Marlow added a comment - 13/Dec/06 09:17 AM

instanceof X will return false if X is null.

[ Permalink | « Hide ]

Scott Marlow added a comment - 13/Dec/06 10:22 AM

I tried modifying org.hibernate.test.bytecode.BytecodeSuite (really org.hibernate.test.bytecode.javassist.ReflectionOptimizerTest.suite()) to recreate this bug but couldn't trigger FastClass.equals.

I know that equals wasn't called because the call to Thread.dumpStack that I added didn't show up in the output. Yes, I'm removing the call to dumpstack. ;)

[ Permalink | « Hide ]

Scott Marlow added a comment - 14/Dec/06 09:07 AM

I ran ./build.sh junitreport with and without the patch. The patch doesn't cause impact the success or failures of any current tests. Its not clear to me if the FastClass.exist is exposed to client code, but lets apply the fix anyway.

Fix checked into trunk revision 10993.

[ Permalink | « Hide ]

Scott Marlow added a comment - 15/Dec/06 10:32 AM

Fix also checked into Branch_3_2 revision 10998.