[jira] [Issue Comment Deleted] (BCEL-195) addition of hashCode() to generic/Instruction.java breaks Targeters

["Sebb (JIRA)" <ji...@apache.org>]

     [ https://issues.apache.org/jira/browse/BCEL-195?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]

Sebb updated BCEL-195:
----------------------
    Comment: was deleted

(was: 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)