[
https://issues.apache.org/jira/browse/BCEL-195?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14715579#comment-14715579
]
Sebb commented on BCEL-195:
---------------------------
I don't follow what you mean about coding guidelines.
The new patch passes all tests including the test case you provided.
However it is a work-round for the fact that the Select object is not properly
constructed.
So there may be other nasties that appear in the future; the behaviour may well
change with updates to JVMs.
So although it works now, it may not be a long-term solution.
I never got a chance to try the original patch because I did not have a test
case at the time.
==
I do now agree that trying to re-use branch instructions is a non-starter with
the current design.
So I wonder why the comparator still tries to compare Select instructions?
Should it still try to compare them?
I just checked, and always returning false for Selects also fixes the new test
case - because it avoids calling getTargets().
Given that the behaviour of getTargets() is effectively undefined during Select
construction, it would be safer not to try comparing them, even with the fix
for the NPE.
It would still be good to fix the construction issue however.
There may be other use cases that fail or behave incorrectly because the Select
object behaves in other unexpected ways.
> 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
> Affects Versions: 6.0
> Reporter: Mark Roberts
> Attachments: compare.diff
>
>
> [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)
