commons-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Sebb (JIRA)" <>
Subject [jira] [Commented] (BCEL-195) addition of hashCode() to generic/ breaks Targeters
Date Wed, 26 Aug 2015 21:49:45 GMT


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
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/ breaks Targeters
> -------------------------------------------------------------------
>                 Key: BCEL-195
>                 URL:
>             Project: Commons BCEL
>          Issue Type: Bug
>          Components: Main
>    Affects Versions: 6.0
>            Reporter: Mark Roberts
>         Attachments: compare.diff
> [Revision 1532198|] 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

View raw message