[
https://issues.apache.org/jira/browse/BCEL-195?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14329160#comment-14329160
]
Emmanuel Bourg commented on BCEL-195:
-------------------------------------
Ok let me recap:
- {{InstructionHandler}} contains a {{Set}} of {{InstructionTarget}}. If the
same targeter is added twice it's referenced only once
- The {{BranchInstruction}} hierarchy implements {{InstructionTarget}} and
inherit the equals/hashCode methods from the base abstract {{Instruction}} class
- Before r1532198 the {{Instruction}} class defined the {{equals()}} method but
not {{hashCode()}}. Thus two instructions that were equal didn't have the same
hashcode. This was a violation of the equals/hashCode contract (equal objects
must have the same hashcode).
- This flaw allowed two distinct but equal instances of a {{BranchInstruction}}
to be listed as targeters of the same {{InstructionHandler}}, because the
{{HashSet}} holding the targeters checks the hashcode before calling
{{equals()}} to detect if it already contains the element.
- The addition of {{Instruction.hashCode()}} in r1532198 fixed this flaw but
had the side effect of "merging" equal but different {{BranchInstructions}} in
the set of targeters.
The issue here is not {{Instruction.hashCode()}} which is correct, but the
{{equals()}} implementation that is wrong. I agree with you that two GOTO 50
shouldn't be considered equal, it doesn't make sense for flow instructions. The
question is, do we even need this {{equals()}} method? What is it really used
for? I removed it and no test complained (but we have a poor coverage). I'm
pondering if I should either remove it (instruction equality can still be
checked with {{InstructionComparator}}) or just remove the code related to
{{BranchInstruction}} from {{InstructionComparator}}.
> addition of hashCode() to generic/Instruction.java breaks Targeters
> -------------------------------------------------------------------
>
> Key: BCEL-195
> URL: https://issues.apache.org/jira/browse/BCEL-195
> Project: Commons BCEL
> Issue Type: Bug
> Components: Main
> Reporter: Mark Roberts
>
> [Revision 1532198|http://svn.apache.org/r1532198] added a {{hashCode()}}
> function to the Instruction class. Unfortunately, this breaks the
> Instruction targeting mechanism. I understand the goal of trying to reuse
> instructions - an 'iadd' is the same as any other 'iadd'. However, one
> 'goto 50' is not the same as another 'goto 50' due to the way Targeters are
> implemented. If branch instructions are reused, then only one entry gets put
> on the Targeter list. So when some api is used to modify the instruction
> list and location 50 becomes location 52 ONLY ONE of the branches gets
> updated. A very bad thing. So unless you modify the hash to special case
> branch instructions (and there might be other instructions needing special
> treatment as well) its broken. We fixed it by simply commenting the hash out
> to make things like they used to be and all works great.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
