Hi Peter,
Yes, they must agree: o1.equals(o2) => o1.hashCode() == o2.hashCode(). I
think that was the original motivation in BCEL 6.0 to add the hashCode()
method since before that, the Instruction class overrode equals() but
not hashCode(), so two Instructions that were equal had different
hashcode. The InstructionComparator compares opcode in general, and adds
conditions for certain subtypes, e.g. BranchInstruction (BI) among
others. What the InstructionComparator does was a complicated matter
originated from how the whole process was designed. They had given it a
lot of thoughts, replacing HashSet among other things for example. The
current form was a compromise reached in BCEL 6.0 after a long
discussion. One such compromise was that BIs were treated uniquely
(unequal), allowing targeters to be added to a HashSet as unique items,
and then be able to be removed by themselves (references). For the later
to happen, the hashcode must remain the same for the same object.
Regards,
Joe
On 6/25/2020 11:53 PM, Peter Levart wrote:
Hi Joe,
For an object to correctly function as an element of a HashSet, its
hashCode() and equals() methods must agree: o1.equals(o2) =>
o1.hashCode() == o2.hashCode(). I see equals() method delegates to a
global InstructionComparator instance which can even change. Suppose
it does not change. But what does such InstructionComparator compare?
opcode (among other things)?
Regards, Peter
On Fri, 26 Jun 2020, 01:56 Joe Wang, <[email protected]
<mailto:[email protected]>> wrote:
Hi,
Please review a fix to a BCEL regression. At issue was the
addition of
hashCode() method to Instruction.java in BCEL 6.0. This hashCode()
method was implemented to return the instruction's opcode that
unfortunately is mutable. The problem hasn't showed up until the code
path led to calls to remove from a HashSet a target that has been
changed since it was added to the HashSet. The proposed patch is
to make
the hashCode final/immutable.
This patch implies that a target object is considered the same one
even
if its field values may have been changed. It therefore may not be
appropriate in other situations (or may cause problems). However,
since
it had always had no hashCode() override before BCEL 6.0, thereby
relying on Object's identity hash code, its use case has been limited
and time-tested. It can benefit from this patch in that it
provides the
same function as Object's hash code, and then serves as a reminder
(to
any who might read the code) how it was used (objects considered
to be
the same over the course as far as the hashCode is concerned).
JBS: https://bugs.openjdk.java.net/browse/JDK-8248348
webrevs: http://cr.openjdk.java.net/~joehw/jdk16/8248348/webrev/
Thanks,
Joe
