Request for review: cleanup in preparation of the final java2d backport.

[Dr Andrew John Hughes]

On 18:03 Tue 01 Mar     , Denis Lila wrote:
> > and attach the patches. I can't really review a patch that isn't
> > attached as I can't comment inline.
> 
> In the future, should I attach the diff, or just attach the
> patches and put the ChangeLog diff in the e-mail?

Include the ChangeLog in the body of the e-mail and not as part
of the patch.  The patch should be an attachment.

Having the ChangeLog as part of the patch makes it difficult for
people to apply and test it.

> 
> > The patch you've separated as 'piscesMakefile.patch' should really be
> > part
> > of whichever patch needs it to make it work. It will need to be
> > upstreamed
> > to OpenJDK6 as one lump to avoid breaking the build.
> 
> Ok. That's what the attached patch does.
> Is it ok to push? 
> 

Yes.

> ChangeLog:
> +2011-03-01  Denis Lila  <dlila at redhat.com>
> +
> +	* Makefile.am:
> +	Removed patch (piscesMakefile.am).
> +	* patches/openjdk/piscesMakefile.patch:
> +	Removed. Its body was added to
> +	6967436-6967433-floating-pt-conversion.patch
> +
> 
> > Why is the amalgamated patch being removed in the first changeset when
> > it is the other changeset that replaces it? IMHO, 1 should just be:
> > 
> > 2011-02-28 Denis Lila <dlila at redhat.com>
> > 
> > * Makefile.am: Replaced patches/renderer-crossing.patch with
> > patches/openjdk/6887494-NPE-in-pisces.patch.
> > * patches/openjdk/6887494-NPE-in-pisces.patch:
> > Added.
> > * patches/renderer-crossing.patch:
> > Removed. Replaced by 6887494-NPE-in-pisces.patch.
> 
> I agree that this isn't very pleasant, but I think doing what
> you suggest above would might break the build since the NPE
> patch came before the huge 3 changeset patch in openjdk7. Moreover,
> the 3 changeset patch assumes that renderer-crossing.patch
> has been applied, so at best we would need to do some complicated
> reordering of the patch application to not break the build. It
> might even be impossible to make it build without removing the
> huge patch, so that's a lot of work for a patch that is removed
> in the next changeset.
> 

I don't follow this.  The two patches are identical so why would
swapping renderer-crossing for 6887494, the upstream version,
break anything?

Anyway, seems you committed so it's too late to change now.

> This is why I put everything in one big changeset when I first
> proposed it...
> 
> > Why does the floating point conversion have two bug IDs?
> 
> Because the commit comment in openjdk7 has 2 bug IDs.

Ah right, ok.

> 
> Thank you,
> Denis.
> 

> diff -r 4573e7757706 ChangeLog
> --- a/ChangeLog	Tue Mar 01 10:33:29 2011 -0500
> +++ b/ChangeLog	Tue Mar 01 17:48:27 2011 -0500
> @@ -1,3 +1,11 @@
> +2011-03-01  Denis Lila  <dlila at redhat.com>
> +
> +	* Makefile.am:
> +	Removed patch (piscesMakefile.am).
> +	* patches/openjdk/piscesMakefile.patch:
> +	Removed. Its body was added to
> +	6967436-6967433-floating-pt-conversion.patch
> +
>  2011-03-01  Denis Lila  <dlila at redhat.com>
>  
>  	* Makefile.am:
> diff -r 4573e7757706 Makefile.am
> --- a/Makefile.am	Tue Mar 01 10:33:29 2011 -0500
> +++ b/Makefile.am	Tue Mar 01 17:48:27 2011 -0500
> @@ -320,7 +320,6 @@
>  	patches/openjdk/6663040-com.sun.awt.patch \
>  	patches/openjdk/6775317-non-AA-simple-shape-performance.patch \
>  	patches/pr600-arm-jvm.cfg.patch \
> -	patches/piscesMakefile.patch \
>  	patches/openjdk/6887494-NPE-in-pisces.patch \
>  	patches/openjdk/6967436-6967433-floating-pt-conversion.patch \
>  	patches/openjdk/6976265-stroke-control.patch \
> diff -r 4573e7757706 patches/openjdk/6967436-6967433-floating-pt-conversion.patch
> --- a/patches/openjdk/6967436-6967433-floating-pt-conversion.patch	Tue Mar 01 10:33:29 2011 -0500
> +++ b/patches/openjdk/6967436-6967433-floating-pt-conversion.patch	Tue Mar 01 17:48:27 2011 -0500
> @@ -1,13 +1,14 @@
> -# HG changeset patch
> -# User dlila
> -# Date 1281460784 14400
> -# Node ID d47bd9d94ba42529aa1adb6d521ebea8fcc00031
> -# Parent  cf0c23a99823536ad6cf720e442896276f8020e5
> -6967436: lines longer than 2^15 can fill window.
> -6967433: dashed lines broken when using scaling transforms.
> -Summary: converted pisces to floating point. Also, using better AA algorithm
> -Reviewed-by: flar
> -
> +--- openjdk/jdk/make/sun/pisces/Makefile.orig  2011-02-23 20:16:27.116261331 -0500
> ++++ openjdk/jdk/make/sun/pisces/Makefile   2011-02-23 20:17:08.106141482 -0500
> +@@ -28,6 +28,8 @@
> + PRODUCT = sun
> + include $(BUILDDIR)/common/Defs.gmk
> + 
> ++LANGUAGE_VERSION = -source 6
> ++CLASS_VERSION = -target 6
> + #
> + # Files
> + #
>  diff -r cf0c23a99823 -r d47bd9d94ba4 src/share/classes/sun/java2d/pisces/Dasher.java
>  --- openjdk.orig/jdk/src/share/classes/sun/java2d/pisces/Dasher.java	Thu Jul 29 17:12:27 2010 -0700
>  +++ openjdk/jdk/src/share/classes/sun/java2d/pisces/Dasher.java	Tue Aug 10 13:19:44 2010 -0400
> diff -r 4573e7757706 patches/piscesMakefile.patch
> --- a/patches/piscesMakefile.patch	Tue Mar 01 10:33:29 2011 -0500
> +++ /dev/null	Thu Jan 01 00:00:00 1970 +0000
> @@ -1,11 +0,0 @@
> ---- openjdk/jdk/make/sun/pisces/Makefile.orig  2011-02-23 20:16:27.116261331 -0500
> -+++ openjdk/jdk/make/sun/pisces/Makefile   2011-02-23 20:17:08.106141482 -0500
> -@@ -28,6 +28,8 @@
> - PRODUCT = sun
> - include $(BUILDDIR)/common/Defs.gmk
> - 
> -+LANGUAGE_VERSION = -source 6
> -+CLASS_VERSION = -target 6
> - #
> - # Files
> - #


-- 
Andrew :)

Free Java Software Engineer
Red Hat, Inc. (http://www.redhat.com)

Support Free Java!
Contribute to GNU Classpath and IcedTea
http://www.gnu.org/software/classpath
http://icedtea.classpath.org
PGP Key: F5862A37 (https://keys.indymedia.org/)
Fingerprint = EA30 D855 D50F 90CD F54D  0698 0713 C3ED F586 2A37