[jira] [Commented] (BCEL-127) InstructionFactory, design issue, createDUP returns static reference

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

    [ https://issues.apache.org/jira/browse/BCEL-127?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14704706#comment-14704706 ] 

Sebb commented on BCEL-127:
---------------------------

URL: http://svn.apache.org/r1696765
Log:
BCEL-127 Document that Instruction Factory returns singleton instances

Modified:
    commons/proper/bcel/trunk/src/main/java/org/apache/commons/bcel6/generic/InstructionFactory.java


> InstructionFactory, design issue, createDUP returns static reference
> --------------------------------------------------------------------
>
>                 Key: BCEL-127
>                 URL: https://issues.apache.org/jira/browse/BCEL-127
>             Project: Commons BCEL
>          Issue Type: Bug
>          Components: Main
>    Affects Versions: 5.2
>         Environment: Operating System: Linux
> Platform: PC
>            Reporter: zagi
>            Assignee: Apache Commons Developers
>
> The implementation of the creation of some instructions e.g. DUPs / NOPs  is really misleading. If you create an invokeinstruction or an constant like ldc a new instruction object is created. Pretty straight forward! But if you create a DUP instruction you always will get the same object back. This is of course is great for saving memory, but in the following example it leads to lots of problems.
> Imagine you instrument a code sequence and add your own instructions one after the other. You add invokestatic instructionsbut also the dups. At the end you iterate again over the whole list to add some missing instructions ( doing it in the first iteration is not possible or too much work, due to the semantics of the algorithm). If you add new instructions again it will not work properly if your insertion point is a DUP! The new instruction will be inserted at the first match of the list.
> e.g.
> i1,... are instructions other than dup,
> dup_ are DUP instructions created with InstructionFactory.createDUP();
> you iterate from i1 to i6;
> init:       i1, i2, i3, dup_1, i4, i5, dup_2, i6
> 1.)  insert(i3, new_i)
> result:  i1, i2, new_i, i3, dup_1, i4, i5, dup_2, i6
> 2.) insert(dup_1, new_i2)
> result:  i1, i2, new_i, i3, new_i2,  dup_1, i4, i5, dup_2, i6
> 3.) insert(dup_2, new_i3)
> result:  i1, i2, new_i, i3, new_i2,  new_i3, dup_1, i4, i5, dup_2, i6
> The reason for this is that dup_1 dup_2 point to the same static DUP instance (there is always one). Your call to insert()
> calls a method findInstruction() and this method only compares references (==) and returns the first match.
> Even if you use copy() you get the same reference! To omit this problem you have to create an instance without the factory by using a constructor.
> My point is, you can implement it this way BUT you have to mention this in the API doc that this createDUP method does not behave like a createInvoke method and always returns the same reference. I know usually the code is the documentation but I think a lot of pain could be spared with a tiny little sentence in the doc.
> My suggestions for the doc of those methods:
> "Returns always a static reference. Note: may cause issues when used as insertion points"
> or something like that



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)