commons-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Thomas Vahrst (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (COLLECTIONS-441) MultiKeyMap.clone() should call super.clone()
Date Sat, 02 Feb 2013 22:06:12 GMT

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

Thomas Vahrst commented on COLLECTIONS-441:
-------------------------------------------

In the suggested solution, the decorated map itself (an AbstractHashedMap instance) is cloned
and then filled into the new MultiKeyMap instance. The clone() method of AbstractHashedMap
contains another findbugs issue: it returns null in the case of a CloneNotSupportedException:

{quote}
org.apache.commons.collections.map.AbstractHashedMap.clone() may return null
{quote}

Instead of returning null, throwing a RuntimeException makes findbugs happy ;-)

{code}
    /**
     * Clones the map without cloning the keys or values.
     * <p>
     * To implement <code>clone()</code>, a subclass must implement the
     * <code>Cloneable</code> interface and make this method public.
     *
     * @return a shallow clone
     */
    @Override
    @SuppressWarnings("unchecked")
    protected AbstractHashedMap<K, V> clone() {
        try {
            final AbstractHashedMap<K, V> cloned = (AbstractHashedMap<K, V>) super.clone();
            cloned.data = new HashEntry[data.length];
            cloned.entrySet = null;
            cloned.keySet = null;
            cloned.values = null;
            cloned.modCount = 0;
            cloned.size = 0;
            cloned.init();
            cloned.putAll(this);
            return cloned;
        } catch (final CloneNotSupportedException ex) {
           // return null; // should never happen.
           throw new RuntimeException(ex); // should never happen. 
        }
    }
{code}
                
> MultiKeyMap.clone() should call super.clone()
> ---------------------------------------------
>
>                 Key: COLLECTIONS-441
>                 URL: https://issues.apache.org/jira/browse/COLLECTIONS-441
>             Project: Commons Collections
>          Issue Type: Improvement
>          Components: Map
>    Affects Versions: 4.0-beta-1
>            Reporter: Thomas Vahrst
>            Priority: Minor
>
> This issue addresses a findbugs issue: 
> {quote}
> org.apache.commons.collections.map.MultiKeyMap.clone() does not call super.clone()
> {quote}
> The current clone() implementation creates a new MultiKeyMap instance. This will lead
to problems when clone() is invoked on subclasses of MultiKeyMap. This is a corresponding
junit test which fails:
> {code}
> class MultiKeyMapTest
>     // Subclass to test clone() method
>     private static class MultiKeyMapSubclass extends MultiKeyMap<String, String>{
>     }
>     public void testCloneSubclass(){
>         MultiKeyMapSubclass m = new MultiKeyMapSubclass();
>         
>         MultiKeyMapSubclass m2 = (MultiKeyMapSubclass) m.clone();
>         
>     }
> {code}
> Instead of creating a new MultiKeyMap instance, the clone() method should invoke super.clone()
which leads in Object.clone(). This always returns an object of the correct type. 
> {code}
> class MultiKeyMap{
>     /**
>      * Clones the map without cloning the keys or values.
>      *
>      * @return a shallow clone
>      */
>     @Override
>     public MultiKeyMap<K, V> clone() {
>        try {
>             MultiKeyMap<K,V> m = (MultiKeyMap<K, V>) super.clone();
>             AbstractHashedMap<MultiKey<? extends K>, V> del = (AbstractHashedMap<MultiKey<?
extends K>, V>)decorated().clone();
>             
>             m.map = del;
>             ((AbstractMapDecorator<K,V>)m).map = (Map) del;
>             return m;
>         } catch (CloneNotSupportedException ex) {
>             throw new RuntimeException (ex);  // this should never happen...
>         }    
>     }
> {code}
> *Note*
> For serialisation compatibilty reasons to commons collections V.3.2,  MultiKeyMap contains
a map reference (the decorated map) which hides the same field in the super class AbstractMapDecorator.
This is quite 'ugly' to understand and maintain.
> Should we consider to break the compatibility to the 3.2 version? 

--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators
For more information on JIRA, see: http://www.atlassian.com/software/jira

Mime
View raw message