Fop
  1. Fop
  2. FOP-1836

[PATCH] Bring clone() in line with the recommendations in Object.clone()

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Resolution: Fixed
    • Affects Version/s: trunk
    • Fix Version/s: None
    • Component/s: unqualified
    • Labels:
      None
    • Environment:
      Operating System: All
      Platform: All
    • External issue ID:
      49754

      Description

      This patch tries to fix the problems with the clone method as reported by findbugs.

      The fix is not straightforward. The current design deviates from the design advocated by findbugs and the documentation of Object.clone(). It is sometimes hard to guess the author's intention with the original clone method. Often the clone is not completely deep; was this intentional or not? This patch makes almost always deep copies, esp. in the subclasses of AreaTreeObject. This patch passes all existing junit tests. Since findbugs requires more memory than my machine has, I could not check if this patch removes all findbugs warnings for clone.

      How does clone differ from a copy constructor? Is it a bad situation to have both a clone method and a copy constructor? Is it advisable to delete one of them?

      Because it is not straightforward, I submit this as a patch, instead of committing it right away.

      1. diff.txt.gz
        6 kB
        Simon Pepping

        Activity

        Hide
        Simon Pepping added a comment -

        Attachment diff.txt.gz has been added with description: patch

        Show
        Simon Pepping added a comment - Attachment diff.txt.gz has been added with description: patch
        Hide
        Andreas L. Delmelle added a comment -

        (In reply to comment #0)
        >
        > How does clone differ from a copy constructor?

        Mainly in the returned type. The result of a call to a copy constructor will always be statically typed as the class where the constructor is defined. This can lead to undesired results if the parameter is an instance of a subclass, unless either:
        a) the copy constructor is overridden in the subclass, which makes it awkward to use vs. a single clone() method, or
        b) the superclass' copy constructor contains some convoluted reflection logic to test the parameter for its runtime type (but that would not take into account possible subclasses out of your control)

        clone(), on the other hand, is guaranteed to always return an instance of Object, whose actual runtime type is (recommended to be) identical to that of the instance for which it is called. In practice, Object.clone() respect this, so having the Cloneable interface is enough for basic shallow cloning to be enabled. The key advantage being that in your code it takes only a single statement to invoke. The Java runtime will sort out which implementation to use.

        > Is it a bad situation to have both a clone method and a copy constructor?
        > Is it advisable to delete one of them?

        Both have their merits. From the point of view of an internal API, if the class itself is final or you know in advance precisely which type(s) you need, a set of copy constructors will generally be more appropriate. From the point of view of an external API, where you need to accommodate potential subclasses, clone() would be the way to go.
        Looking closer at the code, especially for the AreaTreeObject subclasses, clone() is only used by the Java2DRenderer, and even then, it is only called externally on PageViewport. All the other calls to clone() happen internally in the area package. There seems to be no hard requirement at all to have a public API to create clones/copies of all the other object classes, so there I would be more inclined to opt for protected copy constructors, or an optimized set of static factory-like methods, maybe...?

        Show
        Andreas L. Delmelle added a comment - (In reply to comment #0) > > How does clone differ from a copy constructor? Mainly in the returned type. The result of a call to a copy constructor will always be statically typed as the class where the constructor is defined. This can lead to undesired results if the parameter is an instance of a subclass, unless either: a) the copy constructor is overridden in the subclass, which makes it awkward to use vs. a single clone() method, or b) the superclass' copy constructor contains some convoluted reflection logic to test the parameter for its runtime type (but that would not take into account possible subclasses out of your control) clone(), on the other hand, is guaranteed to always return an instance of Object, whose actual runtime type is (recommended to be) identical to that of the instance for which it is called. In practice, Object.clone() respect this, so having the Cloneable interface is enough for basic shallow cloning to be enabled. The key advantage being that in your code it takes only a single statement to invoke. The Java runtime will sort out which implementation to use. > Is it a bad situation to have both a clone method and a copy constructor? > Is it advisable to delete one of them? Both have their merits. From the point of view of an internal API, if the class itself is final or you know in advance precisely which type(s) you need, a set of copy constructors will generally be more appropriate. From the point of view of an external API, where you need to accommodate potential subclasses, clone() would be the way to go. Looking closer at the code, especially for the AreaTreeObject subclasses, clone() is only used by the Java2DRenderer, and even then, it is only called externally on PageViewport. All the other calls to clone() happen internally in the area package. There seems to be no hard requirement at all to have a public API to create clones/copies of all the other object classes, so there I would be more inclined to opt for protected copy constructors, or an optimized set of static factory-like methods, maybe...?
        Hide
        Glenn Adams added a comment -

        resetting P2 open bugs to P3 pending further review

        Show
        Glenn Adams added a comment - resetting P2 open bugs to P3 pending further review
        Hide
        Glenn Adams added a comment -

        lower priority to P5, since this patch would not have an impact on features or performance

        Show
        Glenn Adams added a comment - lower priority to P5, since this patch would not have an impact on features or performance
        Hide
        Glenn Adams added a comment -

        increase priority due to presence of a patch

        Show
        Glenn Adams added a comment - increase priority due to presence of a patch
        Hide
        Glenn Adams added a comment -

        patch landed at http://svn.apache.org/viewvc?view=revision&revision=1311120 with a few minor changes; N.B. IFGraphicContext.clone was unable to employ super.clone() at this time, so this remains a findbugs exclusion for now;

        thanks simon!

        Show
        Glenn Adams added a comment - patch landed at http://svn.apache.org/viewvc?view=revision&revision=1311120 with a few minor changes; N.B. IFGraphicContext.clone was unable to employ super.clone() at this time, so this remains a findbugs exclusion for now; thanks simon!
        Hide
        Luis Bernardo added a comment -

        this patch introduced FOP-2138 for which there already a patch (attachment 29465) that fixes it. looking at it the question I have is whether MainReference should also implement its own clone() method which then would be called by the BodyRegion.clone().

        Show
        Luis Bernardo added a comment - this patch introduced FOP-2138 for which there already a patch (attachment 29465) that fixes it. looking at it the question I have is whether MainReference should also implement its own clone() method which then would be called by the BodyRegion.clone().

          People

          • Assignee:
            fop-dev
            Reporter:
            Simon Pepping
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development