clirr-devel — Discussion about the source code of Clirr

[Clirr-devel] CVS: clirr/src/test/net/sf/clirr/event MessageTest.java,NONE,1.1

[Simon K. <s_k...@us...>]

Update of /cvsroot/clirr/clirr/src/test/net/sf/clirr/event
In directory sc8-pr-cvs1.sourceforge.net:/tmp/cvs-serv3450/src/test/net/sf/clirr/event

Added Files:
	MessageTest.java 
Log Message:
Message translation framework


--- NEW FILE ---
package net.sf.clirr.event;

import java.util.Locale;
import junit.framework.TestCase;

/**
 * Tests for the Message and MessageManager classes.
 * <p>
 * It is assumed here that the other unit tests have forced every Check
 * class to be loaded into memory, hence all the static Message objects
 * have been created and registered with the MessageManager.
 */
public class MessageTest extends TestCase
{
    /**
     * This test verifies that none of the check classes has used
     * a message-id which is already in use elsewhere. It is assumed
     * that the other unit tests will already have caused every available
     * check class to be loaded into memory, therefore causing all the
     * static Message objects to be created.
     */
    public void testUnique()
    {
        MessageManager.getInstance().checkUnique();
    }

    /**
     * This test verifies that the default resource bundle contains an
     * entry for every known message.
     * <p>
     * Unfortunately, it is not possible to check whether, for example,
     * the "de" locale has a complete set of translations. This is because
     * the ResourceBundle implementation simply returns a string from an
     * inherited "parent" resource bundle if the key is not found in a
     * locale-specific bundle, and there is no way of telling which
     * bundle the message was retrieved from.
     */
    public void testComplete()
    {
        // check the english locale
        MessageManager.getInstance().setLocale(new Locale("en"));
        MessageManager.getInstance().checkComplete();
    }
}

[Clirr-devel] CVS: clirr/src/test/net/sf/clirr/event - New directory

[Simon K. <s_k...@us...>]

Update of /cvsroot/clirr/clirr/src/test/net/sf/clirr/event
In directory sc8-pr-cvs1.sourceforge.net:/tmp/cvs-serv3303/src/test/net/sf/clirr/event

Log Message:
Directory /cvsroot/clirr/clirr/src/test/net/sf/clirr/event added to the repository

[Clirr-devel] CVS: clirr/src/java/net/sf/clirr/checks MethodSetCheck.java,1.13,1.14

[Simon K. <s_k...@us...>]

Update of /cvsroot/clirr/clirr/src/java/net/sf/clirr/checks
In directory sc8-pr-cvs1.sourceforge.net:/tmp/cvs-serv27733

Modified Files:
	MethodSetCheck.java 
Log Message:
Rewrite reportMethodAdded method; should be no logic change, just minor
change to report message.


Index: MethodSetCheck.java
===================================================================
RCS file: /cvsroot/clirr/clirr/src/java/net/sf/clirr/checks/MethodSetCheck.java,v
retrieving revision 1.13
retrieving revision 1.14
diff -u -r1.13 -r1.14
--- MethodSetCheck.java	23 Jun 2004 04:57:43 -0000	1.13
+++ MethodSetCheck.java	28 Jun 2004 05:58:13 -0000	1.14
@@ -422,15 +422,27 @@
      */
     private void reportMethodAdded(JavaClass newClass, Method newMethod)
     {
-
-        final Severity severity = !newClass.isInterface() && (newClass.isFinal() || !newMethod.isAbstract())
-                ? Severity.INFO
-                : Severity.ERROR;
-
-        fireDiff("Method '"
-                + getMethodId(newClass, newMethod)
-                + "' has been added",
-                severity, newClass, newMethod);
+        if (newClass.isInterface())
+        {
+            fireDiff("Method '"
+                    + getMethodId(newClass, newMethod)
+                    + "' has been added to an interface",
+                    Severity.ERROR, newClass, newMethod);
+        }
+        else if (newMethod.isAbstract())
+        {
+            fireDiff("Abstract method '"
+                    + getMethodId(newClass, newMethod)
+                    + "' has been added",
+                    Severity.ERROR, newClass, newMethod);
+        }
+        else
+        {
+            fireDiff("Method '"
+                    + getMethodId(newClass, newMethod)
+                    + "' has been added",
+                    Severity.INFO, newClass, newMethod);
+        }
     }
 
     /**

Re: [Clirr-devel] adding methods to interfaces

[Simon K. <si...@ec...>]

On Mon, 2004-06-28 at 17:54, Lars K=FChne wrote:
> Simon Kitching wrote:
>=20
> >>
> >
> >The issue is that binary incompatibilities are *always* failures; just
> >having two incompatible classes together in the classpath causes a
> >failure.
> >
> > =20
> >
>=20
> You mean "having two incompatible classes in the same classloader", rig=
ht?

Yep.=20

>=20
> >For "warning" level issues, the app *may* run or *may* cause a runtime
> >exception depending on exactly how the classes are used.
> > =20
> >
>=20
> Hmmm... I think the situation for commons-collections is a bit special.=
=20
> I like to think of binary compatibility as "binary compatible in all=20
> possible scenarios".
>=20
>  From the Eclipse paper: "Adding a new method to an API interface that=20
> is implemented by Clients (e.g., a callback, listener, or visitor=20
> interface) breaks compatibility because hypothetical pre-existing=20
> implementations do not implement the new method."
>=20
> There are many libraries where users are actually expected to provide=20
> interface implementations. Think PropertyChangeListener in the JDK.=20
> Think Appender in log4j. Think all kinds of Decorators. Clirr can't fin=
d=20
> out whether the interface is meant to be implemented externally (maybe=20
> in JDK 1.5 some class metadata could be used to express that...)
>=20
> My current thinking is this should be flagged as an error, and clirr=20
> users who modify interfaces that are typically not implemented by a use=
r=20
> (like in commons-collection) can somehow filter those messages.
>=20
> The effect is that if you introduce new methods in an interface you are=
=20
> forced to make a decision and actively approve the change. You cannot=20
> ignore the error (like you can ignore a warning) because it will break=20
> your build.

Ok, I'm happy with that as long as we can document this, so when people
see ERROR being reported on their interfaces they know why, and know
that we know that it is "probably" an error, not "definitely" one.

The new message framework (I18N) should help, as messages will now have
codes that can link to further explanations.

> >>
> >>There are theoretically four combinations of final and abstract. One =
of=20
> >>them (abstract method in a non-final class) creates a compatibility=20
> >>problem and the code reports that correctly.
> >>
> >>The remaining three cases do not create a compatibility problem. One =
of=20
> >>those remaining three cases (abstract method and final class) cannot =
be=20
> >>achieved with a correct java compiler - but that doesn't matter here,=
=20
> >>because it is still not a compatibility problem: there can't be any=20
> >>subclasses where the method is missing because the class is final.=20
> >>Reporting INFO in that case is OK, no?
> > =20
> >
>=20
> Yep, change the code as you like, I think we agree on the calculation=20
> result for the relevant three cases :-)

I see:
  non-final + non-abstract --> info
  non-final + abstract --> ERROR
  final + non-abstract --> info
  final + abstract --> impossible

ie the final/non-final state of the class is irrelevant.

I have just committed a patch to MethodSetCheck to rewrite the "reportAdd=
ed" method using if-stmts. I hope you can check this for me.


> PS: will you start working on the i18n improvements you suggested or=20
> should I do it?

I'm right in the middle of committing some patches now - literally.

PS: I'm logged in to IRC if you wish to catch up. Of course, design
discussions may well be better held by email so they are archived.

Cheers,

Simon

Re: [Clirr-devel] adding methods to interfaces

[<lak...@t-...>]

Simon Kitching wrote:

>On Fri, 2004-06-25 at 17:53, Lars Kühne wrote:
>  
>
>>Simon Kitching wrote:
>>
>>    
>>
>>>Hi,
>>>
>>>In MethodSetCheck, the reportMethodAdded method uses:
>>>     final Severity severity = 
>>>	!newClass.isInterface() && 
>>>	(newClass.isFinal() || !newMethod.isAbstract())
>>>               ? Severity.INFO
>>>               : Severity.ERROR;
>>>
>>>
>>>This means that adding a method to an interface is reported as an ERROR.
>>>However the java spec section 13.5.3 says:
>>>
>>><quote>
>>>Adding a method to an interface does not break compatibility with
>>>pre-existing binaries
>>></quote>
>>>
>>>I've tested this, and this seems to be correct: there will not be a
>>>*linktime* error reported under any circumstances. However if I have
>>>code which implements the old interface, and I then pass this object to
>>>code that tries to invoke a new method, an AbstractMethodError runtime
>>>exception occurs.
>>>
>>>So I think that this should be a WARNING, not an error. What do you
>>>think?
>>>
>>> 
>>>
>>>      
>>>
>>I think we could follow the same route that I'm currently investigating 
>>for source incompatibilies. I.e. the ApiDifference should not have only 
>>one severity field, but one field for each compatibility class 
>>(linktime, runtime, source).
>>
>>In the meantime I think ERROR is OK - "clirr" stands for "//Client 
>>binaries must link and *run* with new //releases...". To me linktime and 
>>runtime errors are pretty much equivalent, either of them means I can't 
>>use the new version as a drop-in replacement because my application will 
>>bomb out with some exception sooner or later.
>>    
>>
>
>The issue is that binary incompatibilities are *always* failures; just
>having two incompatible classes together in the classpath causes a
>failure.
>
>  
>

You mean "having two incompatible classes in the same classloader", right?

>For "warning" level issues, the app *may* run or *may* cause a runtime
>exception depending on exactly how the classes are used.
>
>The issue really became obvious for me when I ran CLIRR against
>commons-collections. Any addition of a method to an interface was
>reported as an error. But code which simply *uses* collections classes
>will work fine with the new release. 
>
>A failure will only occur for code which *subclasses* the interfaces
>present in the old release then passes a subclass instance to some code
>which attempts to use a method added in the new release. This is *not*
>normal for users of commons-collections. So despite the fact that about
>30 "ERROR" messages were generated by clirr, the commons-collections
>release is perfectly "compatible" for its normal users.
>
>  
>

Hmmm... I think the situation for commons-collections is a bit special. 
I like to think of binary compatibility as "binary compatible in all 
possible scenarios".

 From the Eclipse paper: "Adding a new method to an API interface that 
is implemented by Clients (e.g., a callback, listener, or visitor 
interface) breaks compatibility because hypothetical pre-existing 
implementations do not implement the new method."

There are many libraries where users are actually expected to provide 
interface implementations. Think PropertyChangeListener in the JDK. 
Think Appender in log4j. Think all kinds of Decorators. Clirr can't find 
out whether the interface is meant to be implemented externally (maybe 
in JDK 1.5 some class metadata could be used to express that...)

My current thinking is this should be flagged as an error, and clirr 
users who modify interfaces that are typically not implemented by a user 
(like in commons-collection) can somehow filter those messages.

The effect is that if you introduce new methods in an interface you are 
forced to make a decision and actively approve the change. You cannot 
ignore the error (like you can ignore a warning) because it will break 
your build.

>>>-------------------------------
>>>
>>>I'm also somewhat puzzled by the remainder of the ternary expression
>>>too.
>>>
>>>Adding an abstract method to a class is indeed a problem, because
>>>subclasses can exist which don't have that method. But like the
>>>interface issue, I think this is a WARNING (runtime problem) rather than
>>>an ERROR (linktime problem).
>>>
>>> 
>>>
>>>      
>>>
>>See above - I think differentiating between linktime and runtime errors 
>>is not very useful from the perspecive of a library user. Let's keep it 
>>at ERROR until we have the extended API available in the ApiDifference 
>>class.
>>
>>    
>>
>>>And why is it ok to add an abstract method to a final class? Final
>>>classes are never allowed to be abstract, so this shouldn't be possible
>>>as far as I know..
>>>
>>> 
>>>
>>>      
>>>
>>Maybe it's because I've just recovered from the flu, but I don't 
>>understand your point here...
>>
>>There are theoretically four combinations of final and abstract. One of 
>>them (abstract method in a non-final class) creates a compatibility 
>>problem and the code reports that correctly.
>>
>>The remaining three cases do not create a compatibility problem. One of 
>>those remaining three cases (abstract method and final class) cannot be 
>>achieved with a correct java compiler - but that doesn't matter here, 
>>because it is still not a compatibility problem: there can't be any 
>>subclasses where the method is missing because the class is final. 
>>Reporting INFO in that case is OK, no?
>>    
>>
>
>As for the interface issue, I think adding an abstract method to a class
>is a WARNING because it only causes problems for *some* users of that
>class. Applications which only instantiate classes which are concrete
>subclasses of the changed class will continue to run fine. This is
>definitely *more likely* to cause problems than the "add method to
>interface" issue discussed above, but there still will be applications
>for which this change does *not* cause a failure of the application when
>the new lib is used with the old app binary.
>
>
>Re the "ternary expression: Ok, so the fact that code reports "INFO" for
>(abstract method + final class) is wrong, but you don't care because
>that situation can never occur? I agree with that, but would prefer to
>use somewhat clearer code to express this; unless you object I will use
>if-statements to make this a bit more obvious to people like me :-)
>
>  
>

Yep, change the code as you like, I think we agree on the calculation 
result for the relevant three cases :-)

Lars

PS: will you start working on the i18n improvements you suggested or 
should I do it?

[Clirr-devel] CVS: clirr/src/test/net/sf/clirr/checks MethodSetCheckTest.java,1.8,1.9

[Simon K. <s_k...@us...>]

Update of /cvsroot/clirr/clirr/src/test/net/sf/clirr/checks
In directory sc8-pr-cvs1.sourceforge.net:/tmp/cvs-serv21443/src/test/net/sf/clirr/checks

Modified Files:
	MethodSetCheckTest.java 
Log Message:
* when scope of method changed, clirr now reports old and new scopes
  rather than "method added" or "method removed".
* grammar fix: it's --> its
* grammar fix: Method -> method


Index: MethodSetCheckTest.java
===================================================================
RCS file: /cvsroot/clirr/clirr/src/test/net/sf/clirr/checks/MethodSetCheckTest.java,v
retrieving revision 1.8
retrieving revision 1.9
diff -u -r1.8 -r1.9
--- MethodSetCheckTest.java	27 Jun 2004 14:21:43 -0000	1.8
+++ MethodSetCheckTest.java	28 Jun 2004 05:09:46 -0000	1.9
@@ -19,7 +19,7 @@
             // method addition and removal
             new ApiDifference("Method 'public void removedMethod(java.lang.String)' has been removed in testlib.MethodsChange",
                     Severity.ERROR, "testlib.MethodsChange", "public void removedMethod(java.lang.String)", null),
-            new ApiDifference("Method 'public int getPriv2()' has been removed in testlib.MethodsChange",
+            new ApiDifference("Accessability of method 'public int getPriv2()' has been decreased from public to private in testlib.MethodsChange",
                     Severity.ERROR, "testlib.MethodsChange", "public int getPriv2()", null),
             new ApiDifference("Method 'protected MethodsChange(int, boolean)' has been added in testlib.MethodsChange",
                     Severity.INFO, "testlib.MethodsChange", "protected MethodsChange(int, boolean)", null),
@@ -35,26 +35,26 @@
                     Severity.INFO, "testlib.AbstractImpl", "public void method()", null),
 
             // Constructor changes
-            new ApiDifference("Parameter 1 of 'protected MethodsChange(int)' has changed it's type to java.lang.Integer in testlib.MethodsChange",
+            new ApiDifference("Parameter 1 of 'protected MethodsChange(int)' has changed its type to java.lang.Integer in testlib.MethodsChange",
                     Severity.ERROR, "testlib.MethodsChange", "protected MethodsChange(int)", null),
 
             // return type changes
-            new ApiDifference("Return type of Method 'public java.lang.Number getPrivAsNumber()' has been changed to java.lang.Integer in testlib.MethodsChange",
+            new ApiDifference("Return type of method 'public java.lang.Number getPrivAsNumber()' has been changed to java.lang.Integer in testlib.MethodsChange",
                     Severity.ERROR, "testlib.MethodsChange", "public java.lang.Number getPrivAsNumber()", null),
             // TODO: INFO if method is final
-            new ApiDifference("Return type of Method 'public java.lang.Integer getPrivAsInteger()' has been changed to java.lang.Number in testlib.MethodsChange",
+            new ApiDifference("Return type of method 'public java.lang.Integer getPrivAsInteger()' has been changed to java.lang.Number in testlib.MethodsChange",
                     Severity.ERROR, "testlib.MethodsChange", "public java.lang.Integer getPrivAsInteger()", null),
 
             // parameter list changes
             // Note: This is the current behaviour, not necessarily the spec of the desired behaviour
             // TODO: need to check assignability of types (and check if method or class is final?)
-            new ApiDifference("In Method 'public void printPriv()' the number of arguments has changed in testlib.MethodsChange",
+            new ApiDifference("In method 'public void printPriv()' the number of arguments has changed in testlib.MethodsChange",
                     Severity.ERROR, "testlib.MethodsChange", "public void printPriv()", null),
-            new ApiDifference("Parameter 1 of 'public void strengthenParamType(java.lang.Object)' has changed it's type to java.lang.String in testlib.MethodsChange",
+            new ApiDifference("Parameter 1 of 'public void strengthenParamType(java.lang.Object)' has changed its type to java.lang.String in testlib.MethodsChange",
                     Severity.ERROR, "testlib.MethodsChange", "public void strengthenParamType(java.lang.Object)", null),
-            new ApiDifference("Parameter 1 of 'public void weakenParamType(java.lang.String)' has changed it's type to java.lang.Object in testlib.MethodsChange",
+            new ApiDifference("Parameter 1 of 'public void weakenParamType(java.lang.String)' has changed its type to java.lang.Object in testlib.MethodsChange",
                     Severity.ERROR, "testlib.MethodsChange", "public void weakenParamType(java.lang.String)", null),
-            new ApiDifference("Parameter 1 of 'public void changeParamType(java.lang.String)' has changed it's type to java.lang.Integer in testlib.MethodsChange",
+            new ApiDifference("Parameter 1 of 'public void changeParamType(java.lang.String)' has changed its type to java.lang.Integer in testlib.MethodsChange",
                     Severity.ERROR, "testlib.MethodsChange", "public void changeParamType(java.lang.String)", null),
 
             // deprecation changes

[Clirr-devel] CVS: clirr/src/conf event-messages.properties,NONE,1.1

[Simon K. <s_k...@us...>]

Update of /cvsroot/clirr/clirr/src/conf
In directory sc8-pr-cvs1.sourceforge.net:/tmp/cvs-serv21029/src/conf

Added Files:
	event-messages.properties 
Log Message:
Message translations for new message infrastructure.


--- NEW FILE ---
# Translations for messages generated by the check classes.
#
# {0} --> full class name of affected class
# {1} --> full method prototype of affected method
# {2} --> full field declaration of affected field
# {3}..{n} --> check-specific parameters

# MethodSetCheck messages
m7000=Method ''{1}'' is now implemented in superclass {3} in {0}
m7001=Abstract method ''{1}'' is now specified by implemented interface {3} in {0}
m7002=Method ''{1}'' has been added in {0}
m7003=Method ''{1}'' has been removed in {0}
m7004=In method ''{1}'' the number of arguments has changed in {0}
m7005=Parameter {3} of ''{1}'' has changed its type to {4} in {0}
m7006=Return type of method ''{1}'' has been changed to {3} in {0}
m7007=Method ''{1}'' has been deprecated in {0}
m7008=Method ''{1}'' is no longer deprecated in {0}
m7009=Accessability of method ''{1}'' has been decreased from {3} to {4} in {0}
m7010=Accessability of method ''{1}'' has been increased from {3} to {4} in {0}

Re: [Clirr-devel] adding methods to interfaces

[Simon K. <si...@ec...>]

On Fri, 2004-06-25 at 17:53, Lars K=FChne wrote:
> Simon Kitching wrote:
>=20
> >Hi,
> >
> >In MethodSetCheck, the reportMethodAdded method uses:
> >      final Severity severity =3D=20
> >	!newClass.isInterface() &&=20
> >	(newClass.isFinal() || !newMethod.isAbstract())
> >                ? Severity.INFO
> >                : Severity.ERROR;
> >
> >
> >This means that adding a method to an interface is reported as an ERRO=
R.
> >However the java spec section 13.5.3 says:
> >
> ><quote>
> >Adding a method to an interface does not break compatibility with
> >pre-existing binaries
> ></quote>
> >
> >I've tested this, and this seems to be correct: there will not be a
> >*linktime* error reported under any circumstances. However if I have
> >code which implements the old interface, and I then pass this object t=
o
> >code that tries to invoke a new method, an AbstractMethodError runtime
> >exception occurs.
> >
> >So I think that this should be a WARNING, not an error. What do you
> >think?
> >
> > =20
> >
>=20
> I think we could follow the same route that I'm currently investigating=
=20
> for source incompatibilies. I.e. the ApiDifference should not have only=
=20
> one severity field, but one field for each compatibility class=20
> (linktime, runtime, source).
>=20
> In the meantime I think ERROR is OK - "clirr" stands for "//Client=20
> binaries must link and *run* with new //releases...". To me linktime an=
d=20
> runtime errors are pretty much equivalent, either of them means I can't=
=20
> use the new version as a drop-in replacement because my application wil=
l=20
> bomb out with some exception sooner or later.

The issue is that binary incompatibilities are *always* failures; just
having two incompatible classes together in the classpath causes a
failure.

For "warning" level issues, the app *may* run or *may* cause a runtime
exception depending on exactly how the classes are used.

The issue really became obvious for me when I ran CLIRR against
commons-collections. Any addition of a method to an interface was
reported as an error. But code which simply *uses* collections classes
will work fine with the new release.=20

A failure will only occur for code which *subclasses* the interfaces
present in the old release then passes a subclass instance to some code
which attempts to use a method added in the new release. This is *not*
normal for users of commons-collections. So despite the fact that about
30 "ERROR" messages were generated by clirr, the commons-collections
release is perfectly "compatible" for its normal users.

>=20
> >-------------------------------
> >
> >I'm also somewhat puzzled by the remainder of the ternary expression
> >too.
> >
> >Adding an abstract method to a class is indeed a problem, because
> >subclasses can exist which don't have that method. But like the
> >interface issue, I think this is a WARNING (runtime problem) rather th=
an
> >an ERROR (linktime problem).
> >
> > =20
> >
>=20
> See above - I think differentiating between linktime and runtime errors=
=20
> is not very useful from the perspecive of a library user. Let's keep it=
=20
> at ERROR until we have the extended API available in the ApiDifference=20
> class.
>=20
> >And why is it ok to add an abstract method to a final class? Final
> >classes are never allowed to be abstract, so this shouldn't be possibl=
e
> >as far as I know..
> >
> > =20
> >
>=20
> Maybe it's because I've just recovered from the flu, but I don't=20
> understand your point here...
>=20
> There are theoretically four combinations of final and abstract. One of=
=20
> them (abstract method in a non-final class) creates a compatibility=20
> problem and the code reports that correctly.
>=20
> The remaining three cases do not create a compatibility problem. One of=
=20
> those remaining three cases (abstract method and final class) cannot be=
=20
> achieved with a correct java compiler - but that doesn't matter here,=20
> because it is still not a compatibility problem: there can't be any=20
> subclasses where the method is missing because the class is final.=20
> Reporting INFO in that case is OK, no?

As for the interface issue, I think adding an abstract method to a class
is a WARNING because it only causes problems for *some* users of that
class. Applications which only instantiate classes which are concrete
subclasses of the changed class will continue to run fine. This is
definitely *more likely* to cause problems than the "add method to
interface" issue discussed above, but there still will be applications
for which this change does *not* cause a failure of the application when
the new lib is used with the old app binary.


Re the "ternary expression: Ok, so the fact that code reports "INFO" for
(abstract method + final class) is wrong, but you don't care because
that situation can never occur? I agree with that, but would prefer to
use somewhat clearer code to express this; unless you object I will use
if-statements to make this a bit more obvious to people like me :-)


Regards,

Simon

Re: [Clirr-devel] ScopeSelector fails on jakarta-commons

[Simon K. <si...@ec...>]

On Mon, 2004-06-28 at 02:14, Lars K=FChne wrote:
> Hi Simon,
>=20
> I'll commit a few changes to ApiDifference that allow differentiating=20
> between src and binary compatibility errors shortly.
>=20
> My smoketests include running clirr against commons-collections, and I=20
> noticed a few of the following messages:
>=20
> ERROR: Unable to find information in class=20
> org.apache.commons.collections.FastHashMap referring back to nested=20
> class=20
> org.apache.commons.collections.FastHashMap$CollectionView$CollectionVie=
wIterator=20
> in old class version
>=20
> I guess the ScopeSelector code has some problems dealing with inner=20
> classes of inner classes. Do you have time to investigate that?

I'd already spotted that and fixed it locally; just forgot to commit the
change. I've now done that.

Regards,

Simon

[Clirr-devel] CVS: clirr/src/java/net/sf/clirr/event ScopeSelector.java,1.2,1.3

[Simon K. <s_k...@us...>]

Update of /cvsroot/clirr/clirr/src/java/net/sf/clirr/event
In directory sc8-pr-cvs1.sourceforge.net:/tmp/cvs-serv12993

Modified Files:
	ScopeSelector.java 
Log Message:
Handle nested-classes-within-nested-classes


Index: ScopeSelector.java
===================================================================
RCS file: /cvsroot/clirr/clirr/src/java/net/sf/clirr/event/ScopeSelector.java,v
retrieving revision 1.2
retrieving revision 1.3
diff -u -r1.2 -r1.3
--- ScopeSelector.java	18 Jun 2004 07:34:48 -0000	1.2
+++ ScopeSelector.java	28 Jun 2004 04:09:34 -0000	1.3
@@ -324,7 +324,7 @@
      */
     public static Scope getClassScope(JavaClass jclass) throws CheckerException
     {
-        int dollarPos = jclass.getClassName().indexOf('$');
+        int dollarPos = jclass.getClassName().lastIndexOf('$');
         if (dollarPos == -1)
         {
             // not a nested class

[Clirr-devel] CVS: clirr/src/test/net/sf/clirr/checks ClassHierarchyCheckTest.java,1.2,1.3 ClassModifierCheckTest.java,1.1,1.2 ClassScopeCheckTest.java,1.1,1.2 FieldSetCheckTest.java,1.6,1.7 GenderChangeCheckTest.java,1.2,1.3

[<lk...@us...>]

Update of /cvsroot/clirr/clirr/src/test/net/sf/clirr/checks
In directory sc8-pr-cvs1.sourceforge.net:/tmp/cvs-serv5056

Modified Files:
	ClassHierarchyCheckTest.java ClassModifierCheckTest.java 
	ClassScopeCheckTest.java FieldSetCheckTest.java 
	GenderChangeCheckTest.java 
Log Message:
declare exception in test methods, so tests can compile after Exception has been added to verify()

Index: ClassHierarchyCheckTest.java
===================================================================
RCS file: /cvsroot/clirr/clirr/src/test/net/sf/clirr/checks/ClassHierarchyCheckTest.java,v
retrieving revision 1.2
retrieving revision 1.3
diff -u -r1.2 -r1.3
--- ClassHierarchyCheckTest.java	23 May 2004 14:57:27 -0000	1.2
+++ ClassHierarchyCheckTest.java	27 Jun 2004 14:39:19 -0000	1.3
@@ -9,7 +9,7 @@
  */
 public class ClassHierarchyCheckTest extends AbstractCheckTestCase
 {
-    public void testHierarchyChangesAreReported()
+    public void testHierarchyChangesAreReported() throws Exception
     {
         ApiDifference[] expected = new ApiDifference[] {
             new ApiDifference("Added java.util.NoSuchElementException to the list of superclasses of testlib.ApplicationException", Severity.WARNING, "testlib.ApplicationException", null, null),

Index: ClassModifierCheckTest.java
===================================================================
RCS file: /cvsroot/clirr/clirr/src/test/net/sf/clirr/checks/ClassModifierCheckTest.java,v
retrieving revision 1.1
retrieving revision 1.2
diff -u -r1.1 -r1.2
--- ClassModifierCheckTest.java	21 Jun 2004 07:24:07 -0000	1.1
+++ ClassModifierCheckTest.java	27 Jun 2004 14:39:19 -0000	1.2
@@ -3,7 +3,6 @@
 import net.sf.clirr.framework.ClassChangeCheck;
 import net.sf.clirr.event.ApiDifference;
 import net.sf.clirr.event.Severity;
-import net.sf.clirr.event.ScopeSelector;
 import net.sf.clirr.framework.ClassSelector;
 
 /**
@@ -11,7 +10,7 @@
  */
 public class ClassModifierCheckTest extends AbstractCheckTestCase
 {
-    public void testAll()
+    public void testAll() throws Exception
     {
         ApiDifference[] expected = new ApiDifference[] {
             new ApiDifference("Added final modifier in class testlib.modifiers.NonFinalBecomesFinal", Severity.ERROR, "testlib.modifiers.NonFinalBecomesFinal", null, null),

Index: ClassScopeCheckTest.java
===================================================================
RCS file: /cvsroot/clirr/clirr/src/test/net/sf/clirr/checks/ClassScopeCheckTest.java,v
retrieving revision 1.1
retrieving revision 1.2
diff -u -r1.1 -r1.2
--- ClassScopeCheckTest.java	18 Jun 2004 07:59:22 -0000	1.1
+++ ClassScopeCheckTest.java	27 Jun 2004 14:39:19 -0000	1.2
@@ -13,21 +13,21 @@
  */
 public class ClassScopeCheckTest extends AbstractCheckTestCase
 {
-    public void testAccessChangesAreReported()
+    public void testAccessChangesAreReported() throws Exception
     {
         ApiDifference[] expected = new ApiDifference[] {
             new ApiDifference("Decreased visibility of class from public to protected", Severity.ERROR, "testlib.scope.ClassScopeChange$A2", null, null),
             new ApiDifference("Decreased visibility of class from public to package", Severity.ERROR, "testlib.scope.ClassScopeChange$A3", null, null),
             new ApiDifference("Decreased visibility of class from public to private", Severity.ERROR, "testlib.scope.ClassScopeChange$A4", null, null),
-            
+
             new ApiDifference("Increased visibility of class from protected to public", Severity.INFO, "testlib.scope.ClassScopeChange$B2", null, null),
             new ApiDifference("Decreased visibility of class from protected to package", Severity.ERROR, "testlib.scope.ClassScopeChange$B3", null, null),
             new ApiDifference("Decreased visibility of class from protected to private", Severity.ERROR, "testlib.scope.ClassScopeChange$B4", null, null),
-            
+
             new ApiDifference("Increased visibility of class from package to public", Severity.INFO, "testlib.scope.ClassScopeChange$C2", null, null),
             new ApiDifference("Increased visibility of class from package to protected", Severity.INFO, "testlib.scope.ClassScopeChange$C3", null, null),
             new ApiDifference("Decreased visibility of class from package to private", Severity.ERROR, "testlib.scope.ClassScopeChange$C4", null, null),
-            
+
             new ApiDifference("Increased visibility of class from private to public", Severity.INFO, "testlib.scope.ClassScopeChange$D2", null, null),
             new ApiDifference("Increased visibility of class from private to protected", Severity.INFO, "testlib.scope.ClassScopeChange$D3", null, null),
             new ApiDifference("Increased visibility of class from private to package", Severity.INFO, "testlib.scope.ClassScopeChange$D4", null, null),

Index: FieldSetCheckTest.java
===================================================================
RCS file: /cvsroot/clirr/clirr/src/test/net/sf/clirr/checks/FieldSetCheckTest.java,v
retrieving revision 1.6
retrieving revision 1.7
diff -u -r1.6 -r1.7
--- FieldSetCheckTest.java	21 Jun 2004 06:59:36 -0000	1.6
+++ FieldSetCheckTest.java	27 Jun 2004 14:39:19 -0000	1.7
@@ -12,7 +12,7 @@
  */
 public class FieldSetCheckTest extends AbstractCheckTestCase
 {
-    public void testFieldCheck()
+    public void testFieldCheck() throws Exception
     {
         ApiDifference[] expected = new ApiDifference[] {
             new ApiDifference("Field stat7 has been removed in testlib.MembersChange", Severity.ERROR, "testlib.MembersChange", null, "stat7"),

Index: GenderChangeCheckTest.java
===================================================================
RCS file: /cvsroot/clirr/clirr/src/test/net/sf/clirr/checks/GenderChangeCheckTest.java,v
retrieving revision 1.2
retrieving revision 1.3
diff -u -r1.2 -r1.3
--- GenderChangeCheckTest.java	27 Dec 2003 19:08:42 -0000	1.2
+++ GenderChangeCheckTest.java	27 Jun 2004 14:39:19 -0000	1.3
@@ -6,7 +6,7 @@
 
 public class GenderChangeCheckTest extends AbstractCheckTestCase
 {
-    public void testGenderChangeCheckTest()
+    public void testGenderChangeCheckTest() throws Exception
     {
         ApiDifference[] expected = new ApiDifference[] {
             new ApiDifference("Changed Gender of testlib.ClassBecomesInterface", Severity.ERROR, "testlib.ClassBecomesInterface", null, null),

[Clirr-devel] CVS: clirr/src/java/net/sf/clirr/ant AntLogger.java,1.6,1.7 AntTask.java,1.12,1.13 ChangeCounter.java,1.4,1.5

[<lk...@us...>]

Update of /cvsroot/clirr/clirr/src/java/net/sf/clirr/ant
In directory sc8-pr-cvs1.sourceforge.net:/tmp/cvs-serv2365/src/java/net/sf/clirr/ant

Modified Files:
	AntLogger.java AntTask.java ChangeCounter.java 
Log Message:
allow differentiating between src and binary compatibility problems

Index: AntLogger.java
===================================================================
RCS file: /cvsroot/clirr/clirr/src/java/net/sf/clirr/ant/AntLogger.java,v
retrieving revision 1.6
retrieving revision 1.7
diff -u -r1.6 -r1.7
--- AntLogger.java	22 May 2004 13:26:03 -0000	1.6
+++ AntLogger.java	27 Jun 2004 14:21:34 -0000	1.7
@@ -44,7 +44,7 @@
 
     public void reportDiff(ApiDifference difference)
     {
-        final Severity severity = difference.getSeverity();
+        final Severity severity = difference.getMaximumSeverity();
         final Integer prio = (Integer) severityPrioMap.get(severity);
         task.log(severity.toString() + ": " + difference.getReport(), prio.intValue());
     }

Index: AntTask.java
===================================================================
RCS file: /cvsroot/clirr/clirr/src/java/net/sf/clirr/ant/AntTask.java,v
retrieving revision 1.12
retrieving revision 1.13
diff -u -r1.12 -r1.13
--- AntTask.java	13 Jun 2004 10:44:25 -0000	1.12
+++ AntTask.java	27 Jun 2004 14:21:34 -0000	1.13
@@ -91,8 +91,10 @@
     private Path newClassPath = null;
     private Path origClassPath = null;
 
-    private boolean failOnError = true;
-    private boolean failOnWarning = false;
+    private boolean failOnBinError = true;
+    private boolean failOnBinWarning = false;
+    private boolean failOnSrcError = true;
+    private boolean failOnSrcWarning = false;
     private List formatters = new LinkedList();
 
 
@@ -156,14 +158,24 @@
         this.newFiles = newFiles;
     }
 
-    public void setFailOnError(boolean failOnError)
+    public void setFailOnBinError(boolean failOnBinError)
     {
-        this.failOnError = failOnError;
+        this.failOnBinError = failOnBinError;
     }
 
-    public void setFailOnWarning(boolean failOnWarning)
+    public void setFailOnBinWarning(boolean failOnBinWarning)
     {
-        this.failOnWarning = failOnWarning;
+        this.failOnBinWarning = failOnBinWarning;
+    }
+
+    public void setFailOnSrcError(boolean failOnSrcError)
+    {
+        this.failOnSrcError = failOnSrcError;
+    }
+
+    public void setFailOnSrcWarning(boolean failOnSrcWarning)
+    {
+        this.failOnSrcWarning = failOnSrcWarning;
     }
 
     public void addFormatter(Formatter formatter)
@@ -253,9 +265,14 @@
             throw new BuildException(ex.getMessage());
         }
 
-        if (counter.getWarnings() > 0 && failOnWarning || counter.getErrors() > 0 && failOnError)
+        if (counter.getBinWarnings() > 0 && failOnBinWarning || counter.getBinErrors() > 0 && failOnBinError)
+        {
+            throw new BuildException("detected binary incompatible API changes");
+        }
+
+        if (counter.getSrcWarnings() > 0 && failOnSrcWarning || counter.getSrcErrors() > 0 && failOnSrcError)
         {
-            throw new BuildException("detected incompatible API changes");
+            throw new BuildException("detected source incompatible API changes");
         }
     }
 

Index: ChangeCounter.java
===================================================================
RCS file: /cvsroot/clirr/clirr/src/java/net/sf/clirr/ant/ChangeCounter.java,v
retrieving revision 1.4
retrieving revision 1.5
diff -u -r1.4 -r1.5
--- ChangeCounter.java	22 May 2004 13:26:03 -0000	1.4
+++ ChangeCounter.java	27 Jun 2004 14:21:34 -0000	1.5
@@ -25,43 +25,79 @@
 
 final class ChangeCounter extends DiffListenerAdapter
 {
-    private int infos = 0;
-    private int warnings = 0;
-    private int errors = 0;
+    private int binInfos = 0;
+    private int binWarnings = 0;
+    private int binErrors = 0;
+
+    private int srcInfos = 0;
+    private int srcWarnings = 0;
+    private int srcErrors = 0;
+
 
     public ChangeCounter()
     {
     }
 
-    public int getInfos()
+    public int getBinInfos()
+    {
+        return binInfos;
+    }
+
+    public int getBinWarnings()
+    {
+        return binWarnings;
+    }
+
+    public int getBinErrors()
+    {
+        return binErrors;
+    }
+
+    public int getSrcInfos()
     {
-        return infos;
+        return srcInfos;
     }
 
-    public int getWarnings()
+    public int getSrcWarnings()
     {
-        return warnings;
+        return srcWarnings;
     }
 
-    public int getErrors()
+    public int getSrcErrors()
     {
-        return errors;
+        return srcErrors;
     }
 
     public void reportDiff(ApiDifference difference)
     {
-        if (Severity.ERROR.equals(difference.getSeverity()))
+        final Severity binSeverity = difference.getBinaryCompatibilitySeverity();
+        if (Severity.ERROR.equals(binSeverity))
         {
-            errors += 1;
+            binErrors += 1;
         }
-        else if (Severity.WARNING.equals(difference.getSeverity()))
+        else if (Severity.WARNING.equals(binSeverity))
         {
-            warnings += 1;
+            binWarnings += 1;
         }
-        else if (Severity.INFO.equals(difference.getSeverity()))
+        else if (Severity.INFO.equals(binSeverity))
         {
-            infos += 1;
+            binInfos += 1;
         }
+
+        final Severity srcSeverity = difference.getSourceCompatibilitySeverity();
+        if (Severity.ERROR.equals(srcSeverity))
+        {
+            srcErrors += 1;
+        }
+        else if (Severity.WARNING.equals(srcSeverity))
+        {
+            srcWarnings += 1;
+        }
+        else if (Severity.INFO.equals(srcSeverity))
+        {
+            srcInfos += 1;
+        }
+
     }
 
 }

[Clirr-devel] CVS: clirr/src/java/net/sf/clirr/event ApiDifference.java,1.10,1.11 PlainDiffListener.java,1.4,1.5 Severity.java,1.5,1.6 XmlDiffListener.java,1.10,1.11

[<lk...@us...>]

Update of /cvsroot/clirr/clirr/src/java/net/sf/clirr/event
In directory sc8-pr-cvs1.sourceforge.net:/tmp/cvs-serv2365/src/java/net/sf/clirr/event

Modified Files:
	ApiDifference.java PlainDiffListener.java Severity.java 
	XmlDiffListener.java 
Log Message:
allow differentiating between src and binary compatibility problems

Index: ApiDifference.java
===================================================================
RCS file: /cvsroot/clirr/clirr/src/java/net/sf/clirr/event/ApiDifference.java,v
retrieving revision 1.10
retrieving revision 1.11
diff -u -r1.10 -r1.11
--- ApiDifference.java	22 Jun 2004 16:52:10 -0000	1.10
+++ ApiDifference.java	27 Jun 2004 14:21:35 -0000	1.11
@@ -32,8 +32,17 @@
     /** human readable change report. */
     private String report;
 
-    /** severity of the change, as determined by clirr. */
-    private Severity severity;
+    /**
+     * severity of the change in terms of binary compatibility,
+     * as determined by clirr.
+     */
+    private Severity binaryCompatibilitySeverity;
+
+    /**
+     * severity of the change in terms of source compatibility,
+     * as determined by clirr.
+     */
+    private Severity sourceCompatibilitySeverity;
 
     /** The fully qualified class name that is affected by the API change. */
     private String affectedClass;
@@ -68,9 +77,9 @@
     /**
      * Create a new API differnce representation.
      *
-     * @param report a human readable string describing the change that was made.
-     * @param severity the severity in terms of binary API compatibility.
-     * @param clazz the fully qualified class name where the change occured
+     * @param report a human readable string describing the change that was made, must be non-null.
+     * @param severity the severity in terms of binary and source code compatibility, must be non-null.
+     * @param clazz the fully qualified class name where the change occured, must be non-null.
      * @param method the method signature of the method that changed, <code>null</code>
      *   if no method was affected.
      * @param field the field name where the change occured, <code>null</code>
@@ -78,24 +87,80 @@
      */
     public ApiDifference(String report, Severity severity, String clazz, String method, String field)
     {
+        this(report, severity, severity, clazz, method, field);
+    }
+
+    /**
+     * Create a new API differnce representation.
+     *
+     * @param report a human readable string describing the change that was made, must be non-null.
+     * @param binarySeverity the severity in terms of binary compatibility, must be non-null.
+     * @param sourceSeverity the severity in terms of source code compatibility, must be non-null.
+     * @param clazz the fully qualified class name where the change occured, must be non-null.
+     * @param method the method signature of the method that changed, <code>null</code>
+     *   if no method was affected.
+     * @param field the field name where the change occured, <code>null</code>
+     *   if no field was affected.
+     */
+    public ApiDifference(String report, Severity binarySeverity, Severity sourceSeverity,
+                         String clazz, String method, String field)
+    {
+        checkNonNull(report);
+        checkNonNull(binarySeverity);
+        checkNonNull(sourceSeverity);
+        checkNonNull(clazz);
+
         this.report = report;
-        this.severity = severity;
+        this.binaryCompatibilitySeverity = binarySeverity;
+        this.sourceCompatibilitySeverity = sourceSeverity;
         this.affectedClass = clazz;
         this.affectedField = field;
         this.affectedMethod = method;
     }
 
+    private void checkNonNull(Object o)
+    {
+        if (o == null)
+        {
+            throw new IllegalArgumentException();
+        }
+    }
+
     /**
-     * The Severity of the API difference. ERROR means that clients will
-     * definately break, WARNING means that clients may break, depending
-     * on how they use the library. See the eclipse paper for further
-     * explanation.
+     * The Severity of the API difference in terms of binary compatibility.
+     * ERROR means that clients will definitely break, WARNING means that
+     * clients may break, depending on how they use the library.
+     * See the eclipse paper for further explanation.
      *
-     * @return the severity of the API difference.
+     * @return the severity of the API difference in terms of binary compatibility.
      */
-    public Severity getSeverity()
+    public Severity getBinaryCompatibilitySeverity()
     {
-        return severity;
+        return binaryCompatibilitySeverity;
+    }
+
+    /**
+     * The Severity of the API difference in terms of source compatibility.
+     * Sometimes this is different than {@link #getBinaryCompatibilitySeverity
+     * binary compatibility severity}, for example adding a checked exception
+     * to a method signature is binary compatible but not source compatible.
+     * ERROR means that clients will definitely break, WARNING means that
+     * clients may break, depending on how they use the library.
+     * See the eclipse paper for further explanation.
+     *
+     * @return the severity of the API difference in terms of source code
+     * compatibility.
+     */
+    public Severity getSourceCompatibilitySeverity()
+    {
+        return sourceCompatibilitySeverity;
+    }
+
+    public Severity getMaximumSeverity()
+    {
+        final Severity src = getSourceCompatibilitySeverity();
+        final Severity bin = getBinaryCompatibilitySeverity();
+        return src.compareTo(bin) < 0 ? bin : src;
     }
 
     /**
@@ -140,7 +205,7 @@
      */
     public String toString()
     {
-        return report + " (" + severity + ") - "
+        return report + " (" + binaryCompatibilitySeverity + ") - "
                 + getAffectedClass() + '[' + getAffectedField() + '/' + getAffectedMethod() + ']';
     }
 
@@ -161,18 +226,24 @@
 
         final ApiDifference other = (ApiDifference) o;
 
-        if (report != null ? !report.equals(other.report) : other.report != null)
+        if (!report.equals(other.report))
         {
             return false;
         }
 
-        if (severity != null ? !severity.equals(other.severity) : other.severity != null)
+        if (!binaryCompatibilitySeverity.equals(other.binaryCompatibilitySeverity))
         {
             return false;
         }
 
+        if (!sourceCompatibilitySeverity.equals(other.sourceCompatibilitySeverity))
+        {
+            return false;
+        }
+
+
         final String otherClass = other.affectedClass;
-        if (affectedClass != null ? !affectedClass.equals(otherClass) : otherClass != null)
+        if (!affectedClass.equals(otherClass))
         {
             return false;
         }
@@ -199,10 +270,13 @@
     {
         int result;
         result = report != null ? report.hashCode() : 0;
-        result = HASHCODE_MAGIC * result + (severity != null ? severity.hashCode() : 0);
-        result = HASHCODE_MAGIC * result + (affectedClass != null ? affectedClass.hashCode() : 0);
+        result = HASHCODE_MAGIC * result + binaryCompatibilitySeverity.hashCode();
+        result = HASHCODE_MAGIC * result + sourceCompatibilitySeverity.hashCode();
+        result = HASHCODE_MAGIC * result + affectedClass.hashCode();
         result = HASHCODE_MAGIC * result + (affectedMethod != null ? affectedMethod.hashCode() : 0);
         result = HASHCODE_MAGIC * result + (affectedField != null ? affectedField.hashCode() : 0);
         return result;
     }
+
+
 }

Index: PlainDiffListener.java
===================================================================
RCS file: /cvsroot/clirr/clirr/src/java/net/sf/clirr/event/PlainDiffListener.java,v
retrieving revision 1.4
retrieving revision 1.5
diff -u -r1.4 -r1.5
--- PlainDiffListener.java	22 May 2004 13:26:03 -0000	1.4
+++ PlainDiffListener.java	27 Jun 2004 14:21:35 -0000	1.5
@@ -34,7 +34,7 @@
     public void reportDiff(ApiDifference difference)
     {
         PrintStream out = getOutputStream();
-        out.print(difference.getSeverity().toString());
+        out.print(difference.getMaximumSeverity().toString());
         out.print(": ");
         out.println(difference.getReport());
     }

Index: Severity.java
===================================================================
RCS file: /cvsroot/clirr/clirr/src/java/net/sf/clirr/event/Severity.java,v
retrieving revision 1.5
retrieving revision 1.6
diff -u -r1.5 -r1.6
--- Severity.java	22 May 2004 13:26:03 -0000	1.5
+++ Severity.java	27 Jun 2004 14:21:35 -0000	1.6
@@ -24,28 +24,37 @@
  *
  * @author lkuehne
  */
-public final class Severity
+public final class Severity implements Comparable
 {
 
     private String representation;
+    private int value;
 
-    private Severity(String representation)
+    private Severity(String representation, int value)
     {
         this.representation = representation;
+        this.value = value;
     }
 
     /** marks an api difference that is binary compatible. */
-    public static final Severity INFO = new Severity("INFO");
+    public static final Severity INFO = new Severity("INFO", 0);
 
     /** marks an api difference that might be binary incompatible. */
-    public static final Severity WARNING = new Severity("WARNING");
+    public static final Severity WARNING = new Severity("WARNING", 1);
 
     /** marks an api difference that is binary incompatible. */
-    public static final Severity ERROR = new Severity("ERROR");
+    public static final Severity ERROR = new Severity("ERROR", 2);
 
     /** @see Object#toString() */
     public String toString()
     {
         return representation;
     }
+
+    /** {@inheritDoc} */
+    public int compareTo(Object o)
+    {
+        Severity other = (Severity) o;
+        return this.value - other.value;
+    }
 }

Index: XmlDiffListener.java
===================================================================
RCS file: /cvsroot/clirr/clirr/src/java/net/sf/clirr/event/XmlDiffListener.java,v
retrieving revision 1.10
retrieving revision 1.11
diff -u -r1.10 -r1.11
--- XmlDiffListener.java	27 May 2004 16:10:50 -0000	1.10
+++ XmlDiffListener.java	27 Jun 2004 14:21:35 -0000	1.11
@@ -43,7 +43,8 @@
     {
         PrintStream out = getOutputStream();
         out.print("  <" + DIFFERENCE);
-        out.print(" severity=\"" + difference.getSeverity() + "\"");
+        out.print(" binseverity=\"" + difference.getBinaryCompatibilitySeverity() + "\"");
+        out.print(" srcseverity=\"" + difference.getSourceCompatibilitySeverity() + "\"");
         out.print(" class=\"" + difference.getAffectedClass() + "\"");
         if (difference.getAffectedMethod() != null)
         {

[Clirr-devel] CVS: clirr/xdocs anttask.xml,1.6,1.7 changes.xml,1.13,1.14 index.xml,1.5,1.6

[<lk...@us...>]

Update of /cvsroot/clirr/clirr/xdocs
In directory sc8-pr-cvs1.sourceforge.net:/tmp/cvs-serv2365/xdocs

Modified Files:
	anttask.xml changes.xml index.xml 
Log Message:
allow differentiating between src and binary compatibility problems

Index: anttask.xml
===================================================================
RCS file: /cvsroot/clirr/clirr/xdocs/anttask.xml,v
retrieving revision 1.6
retrieving revision 1.7
diff -u -r1.6 -r1.7
--- anttask.xml	2 Jun 2004 07:53:31 -0000	1.6
+++ anttask.xml	27 Jun 2004 14:21:43 -0000	1.7
@@ -61,14 +61,30 @@
                     <td>Default</td>
                 </tr>
                 <tr>
-                    <td>failOnWarning</td>
-                    <td>Whether task execution should fail (break the build) on warnings</td>
+                    <td>failOnBinWarning</td>
+                    <td>Whether task execution should fail (break the build) on warnings
+                        about binary compatibility issues</td>
                     <td>No</td>
                     <td>No</td>
                 </tr>
                 <tr>
-                    <td>failOnError</td>
-                    <td>Whether task execution should fail (break the build) on errors</td>
+                    <td>failOnBinError</td>
+                    <td>Whether task execution should fail (break the build) on binary
+                        compatibility errors</td>
+                    <td>No</td>
+                    <td>Yes</td>
+                </tr>
+                <tr>
+                    <td>failOnSrcWarning</td>
+                    <td>Whether task execution should fail (break the build) on warnings
+                        about source code compatibility issues</td>
+                    <td>No</td>
+                    <td>No</td>
+                </tr>
+                <tr>
+                    <td>failOnSrcError</td>
+                    <td>Whether task execution should fail (break the build) on source
+                        compatibility errors</td>
                     <td>No</td>
                     <td>Yes</td>
                 </tr>

Index: changes.xml
===================================================================
RCS file: /cvsroot/clirr/clirr/xdocs/changes.xml,v
retrieving revision 1.13
retrieving revision 1.14
diff -u -r1.13 -r1.14
--- changes.xml	21 Jun 2004 10:07:10 -0000	1.13
+++ changes.xml	27 Jun 2004 14:21:43 -0000	1.14
@@ -42,6 +42,11 @@
             which has no public or protected constructors, as it was always
             impossible to derive subclasses from it anyway.
         </action>
+        <action dev="lkuehne" type="add">
+            Clirr now analyses code changes for source code compatibility problems as well.
+            Note: Ant task attribute names and the output format of the XML formatter
+            have changed to support this feature.
+        </action>
     </release>
 
     <release version="0.3" date="2004-05-23">

Index: index.xml
===================================================================
RCS file: /cvsroot/clirr/clirr/xdocs/index.xml,v
retrieving revision 1.5
retrieving revision 1.6
diff -u -r1.5 -r1.6
--- index.xml	20 May 2004 13:28:02 -0000	1.5
+++ index.xml	27 Jun 2004 14:21:43 -0000	1.6
@@ -9,12 +9,12 @@
     <body>
         <section name="What is it?">
             <p>
-Clirr is a tool that checks Java libraries for binary compatibility
+Clirr is a tool that checks Java libraries for binary and source compatibility
 with older releases. Basically you give it two
 sets of jar files and Clirr dumps out a list of changes in the public api.
 The Clirr Ant task can be configured to break the build if it detects
 incompatible api changes. In a continuous integration process Clirr can
-automatically prevent accidental introduction of binary compatibility
+automatically prevent accidental introduction of binary or source compatibility
 problems.
             </p>
         </section>
@@ -40,7 +40,8 @@
 
         <section name="Features">
             <ul>
-                <li>Report all binary API changes (currently only partially implemented)</li>
+                <li>Report all API changes (currently only partially implemented)</li>
+                <li>Evaluate each change wrt. binary and source compatibility</li>
                 <li>support plain text and XML reports</li>
                 <li>Flexible failure handling (warnings vs. errors, break the build or set error property)</li>
             </ul>

[Clirr-devel] CVS: clirr/src/test/net/sf/clirr/checks MethodSetCheckTest.java,1.7,1.8

[<lk...@us...>]

Update of /cvsroot/clirr/clirr/src/test/net/sf/clirr/checks
In directory sc8-pr-cvs1.sourceforge.net:/tmp/cvs-serv2365/src/test/net/sf/clirr/checks

Modified Files:
	MethodSetCheckTest.java 
Log Message:
allow differentiating between src and binary compatibility problems

Index: MethodSetCheckTest.java
===================================================================
RCS file: /cvsroot/clirr/clirr/src/test/net/sf/clirr/checks/MethodSetCheckTest.java,v
retrieving revision 1.7
retrieving revision 1.8
diff -u -r1.7 -r1.8
--- MethodSetCheckTest.java	23 Jun 2004 02:10:17 -0000	1.7
+++ MethodSetCheckTest.java	27 Jun 2004 14:21:43 -0000	1.8
@@ -12,7 +12,7 @@
  */
 public class MethodSetCheckTest extends AbstractCheckTestCase
 {
-    public void testMethodCheck()
+    public void testMethodCheck() throws Exception
     {
         ApiDifference[] expected = new ApiDifference[] {
 

[Clirr-devel] CVS: clirr/src/test/net/sf/clirr/ant ChangeCounterTest.java,1.3,1.4

[<lk...@us...>]

Update of /cvsroot/clirr/clirr/src/test/net/sf/clirr/ant
In directory sc8-pr-cvs1.sourceforge.net:/tmp/cvs-serv2365/src/test/net/sf/clirr/ant

Modified Files:
	ChangeCounterTest.java 
Log Message:
allow differentiating between src and binary compatibility problems

Index: ChangeCounterTest.java
===================================================================
RCS file: /cvsroot/clirr/clirr/src/test/net/sf/clirr/ant/ChangeCounterTest.java,v
retrieving revision 1.3
retrieving revision 1.4
diff -u -r1.3 -r1.4
--- ChangeCounterTest.java	27 Dec 2003 19:08:42 -0000	1.3
+++ ChangeCounterTest.java	27 Jun 2004 14:21:35 -0000	1.4
@@ -15,8 +15,8 @@
       counter.reportDiff(new ApiDifference("blah", Severity.ERROR, "Test", null, null));
       counter.reportDiff(new ApiDifference("blah", Severity.ERROR, "Test", null, null));
       counter.reportDiff(new ApiDifference("blah", Severity.WARNING, "Test", null, null));
-      assertEquals("number of expected errors", 3, counter.getErrors());
-      assertEquals("number of expected warnings", 2, counter.getWarnings());
-      assertEquals("number of expected infos", 1, counter.getInfos());
+      assertEquals("number of expected errors", 3, counter.getBinErrors());
+      assertEquals("number of expected warnings", 2, counter.getBinWarnings());
+      assertEquals("number of expected infos", 1, counter.getBinInfos());
   }
 }
\ No newline at end of file

[Clirr-devel] ScopeSelector fails on jakarta-commons

[<lak...@t-...>]

Hi Simon,

I'll commit a few changes to ApiDifference that allow differentiating 
between src and binary compatibility errors shortly.

My smoketests include running clirr against commons-collections, and I 
noticed a few of the following messages:

ERROR: Unable to find information in class 
org.apache.commons.collections.FastHashMap referring back to nested 
class 
org.apache.commons.collections.FastHashMap$CollectionView$CollectionViewIterator 
in old class version

I guess the ScopeSelector code has some problems dealing with inner 
classes of inner classes. Do you have time to investigate that?

Lars

Re: [Clirr-devel] message strings in reports

[<lak...@t-...>]

Simon Kitching wrote:

>Hi,
>
>Currently, when api differences are reported, the check generating the
>report manually builds the difference "description" string. This has a
>couple of problems.
>
>Firstly, multi-language support can't be done. It would be nice to be
>able to provide clirr reports in languages other than english 
>

+1

Personally I would like to see an option to enforce english messages, as 
usually all other dev tools I use are not localized (IDE, Java compiler, 
etc.). I find switching between languages harder than using a foreign 
language consistently - but maybe that's just me.

Technically this is no problem, we have that in Checkstyle.

>(German
>and French translations shouldn't be too hard to arrange :-).
>
>  
>

:-)

>More importantly, the current messages tend to be terse. In many cases,
>I think it would be desirable for differences to be able to reference a
>longer description of the problem, eg "for more info, see issue #7 in
>the clirr docs", where clirr documentation has a couple of paragraphs
>explaining why issue #7 is a binary incompatibility, or why it is a
>warning and under what circumstances problems can occur.
>
>  
>

Yes, very good idea!

>I propose that some kind of "ReportMessage" class be created that is
>capable of reading messages from an external resource file. When checks
>report differences, they indicate just "Message #7" rather than build
>the exact string.
>

Or maybe use message keys like "removedMethod" instead of just numbers? 
That might make maintainance of resource bundles a bit easier. We could 
also introduce some constants (REMOVED_METHOD) in the ApiDifference 
class... we'll see, I'm sure we can work out those details later.

> The ApiDifference class can then use the ReportMessage
>class to build the "short description" (potentially using the standard
>Java string interpolation syntax of {0} and {1} to insert class and
>method/field names). The message code can also be used to reference an
>external doc that describes the problem in more detail.
>
>
>Thoughts?
>
>  
>

Sounds great!

Lars

Re: [Clirr-devel] adding methods to interfaces

[<lak...@t-...>]

Simon Kitching wrote:

>Hi,
>
>In MethodSetCheck, the reportMethodAdded method uses:
>      final Severity severity = 
>	!newClass.isInterface() && 
>	(newClass.isFinal() || !newMethod.isAbstract())
>                ? Severity.INFO
>                : Severity.ERROR;
>
>
>This means that adding a method to an interface is reported as an ERROR.
>However the java spec section 13.5.3 says:
>
><quote>
>Adding a method to an interface does not break compatibility with
>pre-existing binaries
></quote>
>
>I've tested this, and this seems to be correct: there will not be a
>*linktime* error reported under any circumstances. However if I have
>code which implements the old interface, and I then pass this object to
>code that tries to invoke a new method, an AbstractMethodError runtime
>exception occurs.
>
>So I think that this should be a WARNING, not an error. What do you
>think?
>
>  
>

I think we could follow the same route that I'm currently investigating 
for source incompatibilies. I.e. the ApiDifference should not have only 
one severity field, but one field for each compatibility class 
(linktime, runtime, source).

In the meantime I think ERROR is OK - "clirr" stands for "//Client 
binaries must link and *run* with new //releases...". To me linktime and 
runtime errors are pretty much equivalent, either of them means I can't 
use the new version as a drop-in replacement because my application will 
bomb out with some exception sooner or later.

>-------------------------------
>
>I'm also somewhat puzzled by the remainder of the ternary expression
>too.
>
>Adding an abstract method to a class is indeed a problem, because
>subclasses can exist which don't have that method. But like the
>interface issue, I think this is a WARNING (runtime problem) rather than
>an ERROR (linktime problem).
>
>  
>

See above - I think differentiating between linktime and runtime errors 
is not very useful from the perspecive of a library user. Let's keep it 
at ERROR until we have the extended API available in the ApiDifference 
class.

>And why is it ok to add an abstract method to a final class? Final
>classes are never allowed to be abstract, so this shouldn't be possible
>as far as I know..
>
>  
>

Maybe it's because I've just recovered from the flu, but I don't 
understand your point here...

There are theoretically four combinations of final and abstract. One of 
them (abstract method in a non-final class) creates a compatibility 
problem and the code reports that correctly.

The remaining three cases do not create a compatibility problem. One of 
those remaining three cases (abstract method and final class) cannot be 
achieved with a correct java compiler - but that doesn't matter here, 
because it is still not a compatibility problem: there can't be any 
subclasses where the method is missing because the class is final. 
Reporting INFO in that case is OK, no?

Cheers,
Lars

[Clirr-devel] message strings in reports

[Simon K. <si...@ec...>]

Hi,

Currently, when api differences are reported, the check generating the
report manually builds the difference "description" string. This has a
couple of problems.

Firstly, multi-language support can't be done. It would be nice to be
able to provide clirr reports in languages other than english (German
and French translations shouldn't be too hard to arrange :-).

More importantly, the current messages tend to be terse. In many cases,
I think it would be desirable for differences to be able to reference a
longer description of the problem, eg "for more info, see issue #7 in
the clirr docs", where clirr documentation has a couple of paragraphs
explaining why issue #7 is a binary incompatibility, or why it is a
warning and under what circumstances problems can occur.

I propose that some kind of "ReportMessage" class be created that is
capable of reading messages from an external resource file. When checks
report differences, they indicate just "Message #7" rather than build
the exact string. The ApiDifference class can then use the ReportMessage
class to build the "short description" (potentially using the standard
Java string interpolation syntax of {0} and {1} to insert class and
method/field names). The message code can also be used to reference an
external doc that describes the problem in more detail.


Thoughts?

Regards,

Simon

[Clirr-devel] adding methods to interfaces

[Simon K. <si...@ec...>]

Hi,

In MethodSetCheck, the reportMethodAdded method uses:
      final Severity severity = 
	!newClass.isInterface() && 
	(newClass.isFinal() || !newMethod.isAbstract())
                ? Severity.INFO
                : Severity.ERROR;


This means that adding a method to an interface is reported as an ERROR.
However the java spec section 13.5.3 says:

<quote>
Adding a method to an interface does not break compatibility with
pre-existing binaries
</quote>

I've tested this, and this seems to be correct: there will not be a
*linktime* error reported under any circumstances. However if I have
code which implements the old interface, and I then pass this object to
code that tries to invoke a new method, an AbstractMethodError runtime
exception occurs.

So I think that this should be a WARNING, not an error. What do you
think?

-------------------------------

I'm also somewhat puzzled by the remainder of the ternary expression
too.

Adding an abstract method to a class is indeed a problem, because
subclasses can exist which don't have that method. But like the
interface issue, I think this is a WARNING (runtime problem) rather than
an ERROR (linktime problem).

And why is it ok to add an abstract method to a final class? Final
classes are never allowed to be abstract, so this shouldn't be possible
as far as I know..

Regards,

Simon

Re: [Clirr-devel] maven goal for executable jar?

[Simon K. <si...@ec...>]

Yep, looks like exactly what I'm looking for. Thanks.

Vincent, if you're around and are willing to add this, it would be much
appreciated. If not, I'll try to overcome my fear of Maven and do this
myself sometime soonish..

That crossworlds library looks like it could be useful in another
situation I'm facing at the moment too. Serendipity...

Just by the way, for email exchanges I *much* prefer "bottom-posting",
where email replies go on the bottom of the preceding email rather than
at the top ("top posting"). It is definitely the convention for all the
commons email lists. [NB: this is just my opinion; feel free to use
whichever you prefer...]

Cheers,

Simon

On Wed, 2004-06-23 at 17:26, Lars K=FChne wrote:
> Never tried it myself, but maybe=20
> http://maven.apache.org/reference/plugins/uberjar/ is what you are=20
> looking for?
>=20
> Simon Kitching wrote:
>=20
> >Hi,
> >
> >Now we have a command-line interface for clirr, do you think it would =
be
> >nice to have a maven goal that builds a jar file which can be used lik=
e
> >this?
> >
> >  java -jar clirr-app.jar  -o old/foo.jar -n new/foo.jar
> >
> >Can maven do this?=20
> >
> >I believe that a jar file can "contain" its dependent jar files (eg
> >commons-cli, bcel, ...) and use something in META-INF to make them
> >effectively "in the classpath". This means that users don't need to
> >download the dependencies and fiddle around with CLASSPATH vars etc...
> >

Re: [Clirr-devel] maven goal for executable jar?

[<lak...@t-...>]

Never tried it myself, but maybe 
http://maven.apache.org/reference/plugins/uberjar/ is what you are 
looking for?

Simon Kitching wrote:

>Hi,
>
>Now we have a command-line interface for clirr, do you think it would be
>nice to have a maven goal that builds a jar file which can be used like
>this?
>
>  java -jar clirr-app.jar  -o old/foo.jar -n new/foo.jar
>
>Can maven do this? 
>
>I believe that a jar file can "contain" its dependent jar files (eg
>commons-cli, bcel, ...) and use something in META-INF to make them
>effectively "in the classpath". This means that users don't need to
>download the dependencies and fiddle around with CLASSPATH vars etc...
>
>Regards,
>
>Simon
>
>
>
>-------------------------------------------------------
>This SF.Net email sponsored by Black Hat Briefings & Training.
>Attend Black Hat Briefings & Training, Las Vegas July 24-29 - 
>digital self defense, top technical experts, no vendor pitches, 
>unmatched networking opportunities. Visit www.blackhat.com
>_______________________________________________
>Clirr-devel mailing list
>Cli...@li...
>https://lists.sourceforge.net/lists/listinfo/clirr-devel
>
>  
>

[Clirr-devel] CVS: clirr/src/java/net/sf/clirr/checks MethodSetCheck.java,1.12,1.13

[Simon K. <s_k...@us...>]

Update of /cvsroot/clirr/clirr/src/java/net/sf/clirr/checks
In directory sc8-pr-cvs1.sourceforge.net:/tmp/cvs-serv5494

Modified Files:
	MethodSetCheck.java 
Log Message:
* simplify way leftover methods are reported.
* also, some checkstyle fixes.


Index: MethodSetCheck.java
===================================================================
RCS file: /cvsroot/clirr/clirr/src/java/net/sf/clirr/checks/MethodSetCheck.java,v
retrieving revision 1.12
retrieving revision 1.13
diff -u -r1.12 -r1.13
--- MethodSetCheck.java	23 Jun 2004 02:07:57 -0000	1.12
+++ MethodSetCheck.java	23 Jun 2004 04:57:43 -0000	1.13
@@ -110,28 +110,20 @@
 
                 filterChangedMethods(
                     baselineMethodName,
-                    compatBaseline, baselineMethods, 
+                    compatBaseline, baselineMethods,
                     currentVersion, currentMethods);
 
-                if (baselineMethods.isEmpty() && currentMethods.isEmpty())
-                {
-                    // ok, all done
-                }
-                else if (baselineMethods.isEmpty())
-                {
-                    reportMethodsAdded(currentVersion, currentMethods);
-                }
-                else if (currentMethods.isEmpty())
+                // if any methods are left, they have no matching method in
+                // the other version, so report as removed or added respectively.
+
+                if (!baselineMethods.isEmpty())
                 {
                     reportMethodsRemoved(compatBaseline, baselineMethods, currentVersion);
                 }
-                else
+
+                if (!currentMethods.isEmpty())
                 {
-                    // error: this should not happen
-                    throw new RuntimeException(
-                        "Internal error in Clirr: one or more methods"
-                        + " on input class [" + compatBaseline.getClassName()
-                        + "] were not correlated.");
+                    reportMethodsAdded(currentVersion, currentMethods);
                 }
             }
         }
@@ -141,7 +133,7 @@
 
     /**
      * Given a list of old and new methods for a particular method name,
-     * find the (old, new) method pairs which have identical argument lists. 
+     * find the (old, new) method pairs which have identical argument lists.
      * <p>
      * For these:
      * <ul>
@@ -149,10 +141,10 @@
      *  <li>remove from the list
      * </ul>
      *
-     * On return from this method, the old and new method lists contain only 
-     * methods whose argument lists have changed between versions [or possibly, 
-     * methods which have been deleted while one or more new methods of the 
-     * same name have been added, depending on how you view it]. All other 
+     * On return from this method, the old and new method lists contain only
+     * methods whose argument lists have changed between versions [or possibly,
+     * methods which have been deleted while one or more new methods of the
+     * same name have been added, depending on how you view it]. All other
      * situations have been dealt with.
      * <p>
      * Note that one or both method lists may be empty on return from
@@ -167,11 +159,11 @@
         for(Iterator bIter = baselineMethods.iterator(); bIter.hasNext(); )
         {
             Method bMethod = (Method) bIter.next();
-            
+
             for(Iterator cIter = currentMethods.iterator(); cIter.hasNext(); )
             {
                 Method cMethod = (Method) cIter.next();
-                
+
                 if (isSoftMatch(bMethod, cMethod))
                 {
                     check(compatBaseline, bMethod, cMethod);
@@ -187,8 +179,8 @@
      * Two methods are a "soft" match if they have the same name and argument
      * list. No two methods on the same class are ever a "soft match" for
      * each other, because the compiler requires distinct parameter lists for
-     * overloaded methods. This also implies that for a given method on an "old" 
-     * class version, there are either zero or one "soft matches" on the new 
+     * overloaded methods. This also implies that for a given method on an "old"
+     * class version, there are either zero or one "soft matches" on the new
      * version.
      * <p>
      * However a "soft match" is not sufficient to ensure binary compatibility.
@@ -196,27 +188,27 @@
      * with code compiled against the previous version of the class.
      * <p>
      * There may also be other differences between methods that are regarded
-     * as "soft matches": the exceptions thrown, the deprecation status of the 
+     * as "soft matches": the exceptions thrown, the deprecation status of the
      * methods, their accessability, etc.
      */
     private boolean isSoftMatch(Method oldMethod, Method newMethod)
     {
         String oldName = oldMethod.getName();
         String newName = newMethod.getName();
-        
+
         if (!oldName.equals(newName))
         {
             return false;
         }
-        
+
         StringBuffer buf = new StringBuffer();
         appendHumanReadableArgTypeList(oldMethod, buf);
         String oldArgs = buf.toString();
-        
+
         buf.setLength(0);
         appendHumanReadableArgTypeList(newMethod, buf);
         String newArgs = buf.toString();
-        
+
         return (oldArgs.equals(newArgs));
     }
 

[Clirr-devel] CVS: clirr/src/test/net/sf/clirr/checks AbstractCheckTestCase.java,1.3,1.4

[Simon K. <s_k...@us...>]

Update of /cvsroot/clirr/clirr/src/test/net/sf/clirr/checks
In directory sc8-pr-cvs1.sourceforge.net:/tmp/cvs-serv4522

Modified Files:
	AbstractCheckTestCase.java 
Log Message:
Don't catch exceptions; allow junit to catch them instead. This means that
the output actually has the real exception cause displayed, making tracking
bugs down much easier..


Index: AbstractCheckTestCase.java
===================================================================
RCS file: /cvsroot/clirr/clirr/src/test/net/sf/clirr/checks/AbstractCheckTestCase.java,v
retrieving revision 1.3
retrieving revision 1.4
diff -u -r1.3 -r1.4
--- AbstractCheckTestCase.java	11 Jun 2004 00:26:36 -0000	1.3
+++ AbstractCheckTestCase.java	23 Jun 2004 04:51:10 -0000	1.4
@@ -46,23 +46,17 @@
     }
 
     protected void verify(ApiDifference[] expected)
+    throws Exception
     {
         TestDiffListener tdl = new TestDiffListener();
         Checker checker = CheckerFactory.createChecker(createCheck(tdl));
         ClassSelector classSelector = createClassSelector();
         
-        try
-        {
-            checker.reportDiffs(
-                getBaseLine(), getCurrent(), 
-                new URLClassLoader(new URL[]{}), 
-                new URLClassLoader(new URL[]{}),
-                classSelector);
-        }
-        catch(Exception ex)
-        {
-            fail("Exception generated:" + ex.getMessage());
-        }
+        checker.reportDiffs(
+            getBaseLine(), getCurrent(), 
+            new URLClassLoader(new URL[]{}), 
+            new URLClassLoader(new URL[]{}),
+            classSelector);
             
         tdl.checkExpected(expected);
     }