[jira] [Comment Edited] (BCEL-207) RemoveLocalVariable(s) doesn't remove the associated Targetters.

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

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

Mark Roberts edited comment on BCEL-207 at 8/17/15 10:04 PM:
-------------------------------------------------------------

The dispose() change causes a problem with my existing code.  But I believe the real issue is the whole shallow vs deep copy thing.  MethodGen.getLocalVariables() returns a shallow copy.  My client code wants to rebuild the LocalVarableTable.  So it calls getLocalVariables to get a copy of the old table and then calls removeLocalVariables to prepare for creating the new table.  But, of course, the new table is a variation of the old so you need to copy some of the items from the old to the new table.  There is/was a subtle assumption that getLocalVariables returned a deep copy in order to do this properly.  It turned out even though the copy was shollow, it worked okay, because removeLocalVariables() didn't really remove anything, just reset the Targeters.  Now dispose really removes things so we have trouble.  I'm not suggesting we remove the dispose stuff.  I would like to discuss just what we mean by a method called getFoo().  Does it return a shallow or deep copy of the Foo object?  I think I am arguing that it should be deep.  This issue gets even messier because some BCEL objects override clone() and some don't.  And some have a copy() method (which I assume is always deep?) and some don't.  (oh boy, now I'm wondering if I should have sent this to the commons list instead.)

Well. as usual, it's more complicated than that.  Turns out sometimes we want getFoo() to be shallow so we can modify the Foo object and sometimes we want getFoo() to be deep so we can create a new Foo from the copy.  So now I'm thinking getFoo() should be shallow, and the user needs to say getFoo().copy() if he wants deep?  What do you think?  (Of course, very few of the BCEL objects have a copy() method....)


was (Author: markro):
The dispose() change causes a problem with my existing code.  But I believe the real issue is the whole shallow vs deep copy thing.  MethodGen.getLocalVariables() returns a shallow copy.  My client code wants to rebuild the LocalVarableTable.  So it calls getLocalVariables to get a copy of the old table and then calls removeLocalVariables to prepare for creating the new table.  But, of course, the new table is a variation of the old so you need to copy some of the items from the old to the new table.  There is/was a subtle assumption that getLocalVariables returned a deep copy in order to do this properly.  It turned out even though the copy was shollow, it worked okay, because removeLocalVariables() didn't really remove anything, just reset the Targeters.  Now dispose really removes things so we have trouble.  I'm not suggesting we remove the dispose stuff.  I would like to discuss just what we mean by a method called getFoo().  Does it return a shallow or deep copy of the Foo object?  I think I am arguing that it should be deep.  This issue gets even messier because some BCEL objects override clone() and some don't.  And some have a copy() method (which I assume is always deep?) and some don't.  (oh boy, now I'm wondering if I should have sent this to the commons list instead.)

> RemoveLocalVariable(s) doesn't remove the associated Targetters.
> ----------------------------------------------------------------
>
>                 Key: BCEL-207
>                 URL: https://issues.apache.org/jira/browse/BCEL-207
>             Project: Commons BCEL
>          Issue Type: Bug
>            Reporter: Mark Roberts
>             Fix For: 6.0
>
>         Attachments: methodgen3.diff
>
>
> RemoveLocalVariable(s) doesn't remove the associated Targetters.



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