clirr-devel — Discussion about the source code of Clirr

[Clirr-devel] CVS: clirr/src/java/net/sf/clirr Checker.java,1.19,1.20

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

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

Modified Files:
	Checker.java 
Log Message:
Fix stupid bug in recent checkin (variable names swapped!).


Index: Checker.java
===================================================================
RCS file: /cvsroot/clirr/clirr/src/java/net/sf/clirr/Checker.java,v
retrieving revision 1.19
retrieving revision 1.20
diff -u -r1.19 -r1.20
--- Checker.java	22 Jun 2004 02:56:59 -0000	1.19
+++ Checker.java	23 Jun 2004 04:45:16 -0000	1.20
@@ -332,14 +332,14 @@
 
             if (compatBaselineClass == null)
             {
-                final String className = compatBaselineClass.getClassName();
+                final String className = currentClass.getClassName();
                 final ApiDifference diff = new ApiDifference(
                     "Added " + className, Severity.INFO, className, null, null);
                 fireDiff(diff);
             }
             else if (currentClass == null)
             {
-                final String className = currentClass.getClassName();
+                final String className = compatBaselineClass.getClassName();
                 final ApiDifference diff = new ApiDifference(
                     "Removed " + className, Severity.ERROR, className, null, null);
                 fireDiff(diff);

[Clirr-devel] CVS: clirr/src/testinput/testlib-v1/testlib MembersChange.java,1.2,1.3

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

Update of /cvsroot/clirr/clirr/src/testinput/testlib-v1/testlib
In directory sc8-pr-cvs1.sourceforge.net:/tmp/cvs-serv3471

Modified Files:
	MembersChange.java 
Log Message:
New test data for unit tests


Index: MembersChange.java
===================================================================
RCS file: /cvsroot/clirr/clirr/src/testinput/testlib-v1/testlib/MembersChange.java,v
retrieving revision 1.2
retrieving revision 1.3
diff -u -r1.2 -r1.3
--- MembersChange.java	23 May 2004 13:52:27 -0000	1.2
+++ MembersChange.java	23 Jun 2004 04:44:25 -0000	1.3
@@ -1,6 +1,6 @@
 package testlib;
 
-public class MembersChange
+public class MembersChange extends BaseMembers
 {
     public static int stat1 = 0;
     public static int stat2 = 0;

[Clirr-devel] CVS: clirr/src/testinput/testlib-v2/testlib BaseMembers.java,NONE,1.1 MembersChange.java,1.3,1.4

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

Update of /cvsroot/clirr/clirr/src/testinput/testlib-v2/testlib
In directory sc8-pr-cvs1.sourceforge.net:/tmp/cvs-serv3386

Modified Files:
	MembersChange.java 
Added Files:
	BaseMembers.java 
Log Message:
New test data for test cases.


--- NEW FILE ---
package testlib;

public class BaseMembers
{
    public static int base1 = 0;
    public int base2 = 0;
}

Index: MembersChange.java
===================================================================
RCS file: /cvsroot/clirr/clirr/src/testinput/testlib-v2/testlib/MembersChange.java,v
retrieving revision 1.3
retrieving revision 1.4
diff -u -r1.3 -r1.4
--- MembersChange.java	23 May 2004 13:52:27 -0000	1.3
+++ MembersChange.java	23 Jun 2004 04:43:49 -0000	1.4
@@ -1,6 +1,6 @@
 package testlib;
 
-public class MembersChange
+public class MembersChange extends BaseMembers
 {
     public static int stat1 = 0;         // same
     public static final int stat2 = 0;   // added final

[Clirr-devel] CVS: clirr/src/testinput/testlib-v1/testlib BaseMembers.java,NONE,1.1

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

Update of /cvsroot/clirr/clirr/src/testinput/testlib-v1/testlib
In directory sc8-pr-cvs1.sourceforge.net:/tmp/cvs-serv3100

Added Files:
	BaseMembers.java 
Log Message:
Additional data for test cases.


--- NEW FILE ---
package testlib;

public class BaseMembers
{
    public static int base1 = 0;
    public int base2 = 0;
}

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

[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-serv12409

Modified Files:
	MethodSetCheckTest.java 
Log Message:
Fix to MethodSetChange class revealed this difference should always have
been reported, but it was not.


Index: MethodSetCheckTest.java
===================================================================
RCS file: /cvsroot/clirr/clirr/src/test/net/sf/clirr/checks/MethodSetCheckTest.java,v
retrieving revision 1.6
retrieving revision 1.7
diff -u -r1.6 -r1.7
--- MethodSetCheckTest.java	13 Jun 2004 11:43:18 -0000	1.6
+++ MethodSetCheckTest.java	23 Jun 2004 02:10:17 -0000	1.7
@@ -21,6 +21,8 @@
                     Severity.ERROR, "testlib.MethodsChange", "public void removedMethod(java.lang.String)", null),
             new ApiDifference("Method 'public int getPriv2()' has been removed 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),
             new ApiDifference("Method 'public java.lang.Long getPrivSquare()' has been added in testlib.MethodsChange",
                     Severity.INFO, "testlib.MethodsChange", "public java.lang.Long getPrivSquare()", null),
 

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

[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-serv11635

Modified Files:
	MethodSetCheck.java 
Log Message:
* add filterSoftMatchedMethods method to find the pairs of methods with
  the same name and exact same argument list before entering the rather
  complex "checkChangedMethods" code. This deals with the simple cases
  in an efficient and obviously-correct manner before the tricker code
  is used to deal with the tricker cases.
* after checkChangedMethods is called, deal with leftover methods. This
  fixes a bug where added/removed methods which have the same name as
  a method that still exists were not being reported.
* rename checkChangedMethods -> filterChangedMethods to be consistent
  with new filterSoftMatchedMethods method
* add convenience methods 'reportMethodsAdded' and 'reportMethodsRemoved'
  which take a list of methods to report on.


Index: MethodSetCheck.java
===================================================================
RCS file: /cvsroot/clirr/clirr/src/java/net/sf/clirr/checks/MethodSetCheck.java,v
retrieving revision 1.11
retrieving revision 1.12
diff -u -r1.11 -r1.12
--- MethodSetCheck.java	23 Jun 2004 00:45:48 -0000	1.11
+++ MethodSetCheck.java	23 Jun 2004 02:07:57 -0000	1.12
@@ -88,24 +88,14 @@
             if (baselineMethodName == null)
             {
                 // a new method name has been added in the new version
-
                 List currentMethods = (List) cNameToMethod.get(currentMethodName);
-                for (Iterator i = currentMethods.iterator(); i.hasNext();)
-                {
-                    Method method = (Method) i.next();
-                    reportMethodAdded(currentVersion, method);
-                }
+                reportMethodsAdded(currentVersion, currentMethods);
             }
             else if (currentMethodName == null)
             {
                 // all methods with name x have been removed from the old version
-
                 List baselineMethods = (List) bNameToMethod.get(baselineMethodName);
-                for (Iterator i = baselineMethods.iterator(); i.hasNext();)
-                {
-                    Method method = (Method) i.next();
-                    reportMethodRemoved(compatBaseline, method, currentVersion);
-                }
+                reportMethodsRemoved(compatBaseline, baselineMethods, currentVersion);
             }
             else
             {
@@ -114,20 +104,141 @@
                 List baselineMethods = (List) bNameToMethod.get(baselineMethodName);
                 List currentMethods = (List) cNameToMethod.get(currentMethodName);
 
-                checkChangedMethods(compatBaseline, baselineMethodName, baselineMethods, currentMethods);
+                filterSoftMatchedMethods(
+                    compatBaseline, baselineMethods,
+                    currentVersion, currentMethods);
+
+                filterChangedMethods(
+                    baselineMethodName,
+                    compatBaseline, baselineMethods, 
+                    currentVersion, currentMethods);
+
+                if (baselineMethods.isEmpty() && currentMethods.isEmpty())
+                {
+                    // ok, all done
+                }
+                else if (baselineMethods.isEmpty())
+                {
+                    reportMethodsAdded(currentVersion, currentMethods);
+                }
+                else if (currentMethods.isEmpty())
+                {
+                    reportMethodsRemoved(compatBaseline, baselineMethods, currentVersion);
+                }
+                else
+                {
+                    // error: this should not happen
+                    throw new RuntimeException(
+                        "Internal error in Clirr: one or more methods"
+                        + " on input class [" + compatBaseline.getClassName()
+                        + "] were not correlated.");
+                }
             }
         }
 
         return true;
     }
 
-    private void checkChangedMethods(
+    /**
+     * Given a list of old and new methods for a particular method name,
+     * find the (old, new) method pairs which have identical argument lists. 
+     * <p>
+     * For these:
+     * <ul>
+     *  <li>report on changes in accessability, return type, etc
+     *  <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 
+     * situations have been dealt with.
+     * <p>
+     * Note that one or both method lists may be empty on return from
+     * this method.
+     */
+    private void filterSoftMatchedMethods(
             JavaClass compatBaseline,
+            List baselineMethods,
+            JavaClass currentVersion,
+            List currentMethods)
+    {
+        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);
+                    bIter.remove();
+                    cIter.remove();
+                    break;
+                }
+            }
+        }
+    }
+
+    /**
+     * 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 
+     * version.
+     * <p>
+     * However a "soft match" is not sufficient to ensure binary compatibility.
+     * A change in the method return type will result in a link error when used
+     * 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 
+     * 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));
+    }
+
+    /**
+     * For each method in the baselineMethods list, find the "best match"
+     * in the currentMethods list, report the changes between this method
+     * pair, then remove both methods from the lists.
+     * <p>
+     * On return, at least one of the method lists will be empty.
+     */
+    private void filterChangedMethods(
             String methodName,
+            JavaClass compatBaseline,
             List baselineMethods,
+            JavaClass currentVersion,
             List currentMethods)
     {
-        while (baselineMethods.size() * currentMethods.size() > 0)
+        // ok, we now have to deal with the tricky cases, where it is not
+        // immediately obvious which old methods correspond to which new
+        // methods.
+
+        while (!baselineMethods.isEmpty() && !currentMethods.isEmpty())
         {
             int[][] similarityTable = buildSimilarityTable(baselineMethods, currentMethods);
 
@@ -244,6 +355,21 @@
     }
 
     /**
+     * Given a list of methods, report each one as being removed.
+     */
+    private void reportMethodsRemoved(
+            JavaClass baselineClass,
+            List baselineMethods,
+            JavaClass currentClass)
+    {
+        for (Iterator i = baselineMethods.iterator(); i.hasNext();)
+        {
+            Method method = (Method) i.next();
+            reportMethodRemoved(baselineClass, method, currentClass);
+        }
+    }
+
+    /**
      * Report that a method has been removed from a class.
      * @param oldClass the class where the method was available
      * @param oldMethod the method that has been removed
@@ -285,6 +411,23 @@
         }
     }
 
+    /**
+     * Given a list of methods, report each one as being added.
+     */
+    private void reportMethodsAdded(
+            JavaClass currentClass,
+            List currentMethods)
+    {
+        for (Iterator i = currentMethods.iterator(); i.hasNext();)
+        {
+            Method method = (Method) i.next();
+            reportMethodAdded(currentClass, method);
+        }
+    }
+
+    /**
+     * Report that a method has been added to a class.
+     */
     private void reportMethodAdded(JavaClass newClass, Method newMethod)
     {
 

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

[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-serv29889

Modified Files:
	MethodSetCheck.java 
Log Message:
Some code rearrangement without changing any functionality.
* now uses CoIterator
* removed checkAddedOrRemoved method; this functionality is now
  inline in the check(...) method.
* code previously in check(...) for building the similarityTable is
  now moved to a new checkChangedMethods method.
* code previously in check(...) related to removed methods has been
  moved to the reportRemovedMethod method.


Index: MethodSetCheck.java
===================================================================
RCS file: /cvsroot/clirr/clirr/src/java/net/sf/clirr/checks/MethodSetCheck.java,v
retrieving revision 1.10
retrieving revision 1.11
diff -u -r1.10 -r1.11
--- MethodSetCheck.java	18 Jun 2004 06:52:10 -0000	1.10
+++ MethodSetCheck.java	23 Jun 2004 00:45:48 -0000	1.11
@@ -25,19 +25,17 @@
 import net.sf.clirr.framework.AbstractDiffReporter;
 import net.sf.clirr.framework.ApiDiffDispatcher;
 import net.sf.clirr.framework.ClassChangeCheck;
+import net.sf.clirr.framework.CoIterator;
 import org.apache.bcel.classfile.JavaClass;
 import org.apache.bcel.classfile.Method;
 import org.apache.bcel.classfile.Attribute;
 import org.apache.bcel.generic.Type;
 
 import java.util.ArrayList;
-import java.util.Collection;
 import java.util.HashMap;
 import java.util.Iterator;
 import java.util.List;
 import java.util.Map;
-import java.util.Set;
-import java.util.TreeSet;
 
 /**
  * Checks the methods of a class.
@@ -78,48 +76,83 @@
         Map bNameToMethod = buildNameToMethodMap(compatBaseline);
         Map cNameToMethod = buildNameToMethodMap(currentVersion);
 
-        checkAddedOrRemoved(bNameToMethod, cNameToMethod, compatBaseline, currentVersion);
+        CoIterator iter = new CoIterator(null, bNameToMethod.keySet(), cNameToMethod.keySet());
 
-        // now the key sets of the two maps are equal,
-        // we only have collections of methods that have the same name
-
-        // for each name analyse the differences
-        for (Iterator it = bNameToMethod.keySet().iterator(); it.hasNext();)
+        while (iter.hasNext())
         {
-            String name = (String) it.next();
+            iter.next();
 
-            List baselineMethods = (List) bNameToMethod.get(name);
-            List currentMethods = (List) cNameToMethod.get(name);
+            String baselineMethodName = (String) iter.getLeft();
+            String currentMethodName = (String) iter.getRight();
 
-            while (baselineMethods.size() * currentMethods.size() > 0)
+            if (baselineMethodName == null)
             {
-                int[][] similarityTable = buildSimilarityTable(baselineMethods, currentMethods);
+                // a new method name has been added in the new version
 
-                int min = Integer.MAX_VALUE;
-                int iMin = baselineMethods.size();
-                int jMin = currentMethods.size();
-                for (int i = 0; i < baselineMethods.size(); i++)
+                List currentMethods = (List) cNameToMethod.get(currentMethodName);
+                for (Iterator i = currentMethods.iterator(); i.hasNext();)
                 {
-                    for (int j = 0; j < currentMethods.size(); j++)
-                    {
-                        final int tableEntry = similarityTable[i][j];
-                        if (tableEntry < min)
-                        {
-                            min = tableEntry;
-                            iMin = i;
-                            jMin = j;
-                        }
-                    }
+                    Method method = (Method) i.next();
+                    reportMethodAdded(currentVersion, method);
                 }
-                Method iMethod = (Method) baselineMethods.remove(iMin);
-                Method jMethod = (Method) currentMethods.remove(jMin);
-                check(compatBaseline, iMethod, jMethod);
+            }
+            else if (currentMethodName == null)
+            {
+                // all methods with name x have been removed from the old version
+
+                List baselineMethods = (List) bNameToMethod.get(baselineMethodName);
+                for (Iterator i = baselineMethods.iterator(); i.hasNext();)
+                {
+                    Method method = (Method) i.next();
+                    reportMethodRemoved(compatBaseline, method, currentVersion);
+                }
+            }
+            else
+            {
+                // assert baselineMethodName equals currentMethodName
+
+                List baselineMethods = (List) bNameToMethod.get(baselineMethodName);
+                List currentMethods = (List) cNameToMethod.get(currentMethodName);
+
+                checkChangedMethods(compatBaseline, baselineMethodName, baselineMethods, currentMethods);
             }
         }
 
         return true;
     }
 
+    private void checkChangedMethods(
+            JavaClass compatBaseline,
+            String methodName,
+            List baselineMethods,
+            List currentMethods)
+    {
+        while (baselineMethods.size() * currentMethods.size() > 0)
+        {
+            int[][] similarityTable = buildSimilarityTable(baselineMethods, currentMethods);
+
+            int min = Integer.MAX_VALUE;
+            int iMin = baselineMethods.size();
+            int jMin = currentMethods.size();
+            for (int i = 0; i < baselineMethods.size(); i++)
+            {
+                for (int j = 0; j < currentMethods.size(); j++)
+                {
+                    final int tableEntry = similarityTable[i][j];
+                    if (tableEntry < min)
+                    {
+                        min = tableEntry;
+                        iMin = i;
+                        jMin = j;
+                    }
+                }
+            }
+            Method iMethod = (Method) baselineMethods.remove(iMin);
+            Method jMethod = (Method) currentMethods.remove(jMin);
+            check(compatBaseline, iMethod, jMethod);
+        }
+    }
+
     private int[][] buildSimilarityTable(List baselineMethods, List currentMethods)
     {
         int[][] similarityTable = new int[baselineMethods.size()][currentMethods.size()];
@@ -157,57 +190,6 @@
     }
 
     /**
-     * Checks for added or removed methods, modifies the argument maps so their key sets are equal.
-     */
-    private void checkAddedOrRemoved(
-            Map bNameToMethod,
-            Map cNameToMethod,
-            JavaClass compatBaseline,
-            JavaClass currentVersion)
-    {
-        // create copies to avoid concurrent modification exception
-        Set baselineNames = new TreeSet(bNameToMethod.keySet());
-        Set currentNames = new TreeSet(cNameToMethod.keySet());
-
-        for (Iterator it = baselineNames.iterator(); it.hasNext();)
-        {
-            String name = (String) it.next();
-            if (!currentNames.contains(name))
-            {
-                Collection removedMethods = (Collection) bNameToMethod.get(name);
-                for (Iterator rmIterator = removedMethods.iterator(); rmIterator.hasNext();)
-                {
-                    Method method = (Method) rmIterator.next();
-                    String methodSignature = getMethodId(compatBaseline, method);
-                    String superClass = findSuperClassWithSignature(methodSignature, currentVersion);
-                    String superInterface = null;
-                    if (method.isAbstract())
-                    {
-                        superInterface = findSuperInterfaceWithSignature(methodSignature, currentVersion);
-                    }
-                    reportMethodRemoved(compatBaseline, method, superClass, superInterface);
-                }
-                bNameToMethod.remove(name);
-            }
-        }
-
-        for (Iterator it = currentNames.iterator(); it.hasNext();)
-        {
-            String name = (String) it.next();
-            if (!baselineNames.contains(name))
-            {
-                Collection addedMethods = (Collection) cNameToMethod.get(name);
-                for (Iterator addIterator = addedMethods.iterator(); addIterator.hasNext();)
-                {
-                    Method method = (Method) addIterator.next();
-                    reportMethodAdded(currentVersion, method);
-                }
-                cNameToMethod.remove(name);
-            }
-        }
-    }
-
-    /**
      * Searches the class hierarchy for a method that has a certtain signature.
      * @param methodSignature the sig we're looking for
      * @param clazz class where search starts
@@ -265,14 +247,21 @@
      * Report that a method has been removed from a class.
      * @param oldClass the class where the method was available
      * @param oldMethod the method that has been removed
-     * @param superClassName the superclass where the method is now available, might be null
+     * @param currentClass the superclass where the method is now available, might be null
      */
     private void reportMethodRemoved(
             JavaClass oldClass,
             Method oldMethod,
-            String superClassName,
-            String superInterfaceName)
+            JavaClass currentClass)
     {
+        String methodSignature = getMethodId(oldClass, oldMethod);
+        String superClassName = findSuperClassWithSignature(methodSignature, currentClass);
+        String superInterfaceName = null;
+        if (oldMethod.isAbstract())
+        {
+            superInterfaceName = findSuperInterfaceWithSignature(methodSignature, currentClass);
+        }
+
         if (superClassName != null)
         {
             fireDiff("Method '"

[Clirr-devel] maven goal for executable jar?

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

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

Re: [Clirr-devel] detect source compatibility problems (was: comments on recent commits)

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

On Wed, 2004-06-23 at 06:33, Lars K=FChne wrote:
> Simon Kitching wrote:
>=20
> >>On a related note, I have recently found out that messing around with=
 a=20
> >>method's declared exceptions will not result in binary compatibility=20
> >>problems (surprising to me). Should we extend clirr's scope to check =
for=20
> >>source compatibility as well?
> >>   =20
> >>
> >
> >I'm +1 on reporting compile-time incompatibilities too.=20
> >
> >I agree that changing the declared exceptions should be reported; a
> >WARNING seems most appropriate to me. What do you think?
> >
> > =20
> >
>=20
> My idea currently is to introduce two severity levels in ApiDeffierence=
,=20
> one for binary compatibility and one for source compatibility. For most=
=20
> differences we detect today they would have the same value.
>=20
> For added checked exceptions binary compatibility severity would be=20
> WARNING (because strange things will happen if the exception is actuall=
y=20
> thrown and the client does not handle it), for removed checked=20
> exceptions it would be INFO. In both cases source compatibility would b=
e=20
> ERROR.
>=20
> This would have some effect on the Ant task, e.g. it should be possible=
=20
> to break the build only on binary errors. I'm currently working on that.
>=20
> You like the idea?

That sounds good to me.

[Clirr-devel] BCEL abstraction API (was: comments on recent commits)

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

Simon Kitching wrote:

>On Mon, 2004-06-21 at 17:50, Lars Kühne wrote:
>  
>
>>Simon Kitching wrote:
>>
>>    
>>
>>>On Mon, 2004-06-21 at 05:13, Lars Kühne wrote:
>>> 
>>>
>>>
>>>So I would be quite keen for clirr to move to ASM sooner or later. BCEL
>>>is working for now, so there's no urgency, but I think it would be a
>>>good idea in the long run. Building a "wrapper" that allows BCEL or ASM
>>>to be "plugged in" is an option, but it would be significant work and I
>>>don't really see any benefits over simply using ASM directly.
>>>
>>> 
>>>
>>>      
>>>
>>    * It would minimize our external dependencies, check implementations
>>      would only reference our own stuff.
>>    * Some IDEs provide their own API for accessing the class info we
>>      need, at least for the current version of the code that is
>>      checked. For example I know that IDEA provides a ProgramStructure
>>      interface (PSI) to access the source code. PSI isn't based on
>>      class files, but it provides all info required for clirr, like
>>      inheritance hierarchy, method signature, etc. Typically the IDE
>>      caches that information, minimizing disk activity. If clirr is
>>      used interactively from an IDE plugin, using PSI instead of
>>      accessing class files could make a real difference in terms of
>>      performance.
>>    
>>
>
>The PSI stuff is a good point.
>
>If IDEA have gone to the effort of designing a nice API for accessing
>class structure, then can we use *that* as our API too? ie write
>adaptors from PSI->BCEL, PSI->ASM, PSI->PSI? It seems silly to invent
>our own API...and presumably APIs are in the public domain (unlike
>implementations).
>  
>

The IDEA license does not allow redistribution of the psi classes (at 
least not in IDEA 3, which is what I have), because they are bundled 
with the idea implementation classes (one jar file).

The Eclipse JDT probably has something similar, but I'm not sure we 
really want to develop against such an API: It's too detailled. I don't 
know much about JDT, but the IDEA psi API gives you access to the actual 
source code of a method, the source file, etc. Clirr does not need all 
that, and it would be really hard to write an adapter that maps BCEL to 
PSI because of all the stuff you have to implement.

In fact I think the API we need would not be very large, it would 
basically consist of the following interfaces:

JavaClass
+ Method[] getMethods()
+ String getName()
+ JavaClass[] getSuperClasses()
+ JavaClass[] getImplementedInterfaces()

Method
+ String getName()
+ String getSignature() // would return sth. like "public void blah()"
+ JavaClass[] getParameterTypes()
+ JavaClass[] getDeclaredExceptions()

Field
+ String getName()
+ JavaClass getType()
+ Object getConstantValue()

Some parts are missing (like access scope), but I think you get the 
idea: It's not very complicated (quite similar to the reflection API), 
it's trivial to implement with BCEL, and I guess it would be trivial to 
implement with ASM, IDEA psi, Eclipse JDT or whatever...

But like you said, I don't think such a refactoring is important near 
term - let's reconsider it when an IDE plugin appears on the horizon...

Lars

[Clirr-devel] detect source compatibility problems (was: comments on recent commits)

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

Simon Kitching wrote:

>>On a related note, I have recently found out that messing around with a 
>>method's declared exceptions will not result in binary compatibility 
>>problems (surprising to me). Should we extend clirr's scope to check for 
>>source compatibility as well?
>>    
>>
>
>I'm +1 on reporting compile-time incompatibilities too. 
>
>I agree that changing the declared exceptions should be reported; a
>WARNING seems most appropriate to me. What do you think?
>
>  
>

My idea currently is to introduce two severity levels in ApiDeffierence, 
one for binary compatibility and one for source compatibility. For most 
differences we detect today they would have the same value.

For added checked exceptions binary compatibility severity would be 
WARNING (because strange things will happen if the exception is actually 
thrown and the client does not handle it), for removed checked 
exceptions it would be INFO. In both cases source compatibility would be 
ERROR.

This would have some effect on the Ant task, e.g. it should be possible 
to break the build only on binary errors. I'm currently working on that.

You like the idea?

Lars

[Clirr-devel] CVS: clirr/src/java/net/sf/clirr/event ApiDifference.java,1.9,1.10

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

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

Modified Files:
	ApiDifference.java 
Log Message:
javadoc

Index: ApiDifference.java
===================================================================
RCS file: /cvsroot/clirr/clirr/src/java/net/sf/clirr/event/ApiDifference.java,v
retrieving revision 1.9
retrieving revision 1.10
diff -u -r1.9 -r1.10
--- ApiDifference.java	22 May 2004 13:26:03 -0000	1.9
+++ ApiDifference.java	22 Jun 2004 16:52:10 -0000	1.10
@@ -70,6 +70,11 @@
      *
      * @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 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 severity, String clazz, String method, String field)
     {
@@ -103,16 +108,28 @@
         return report;
     }
 
+    /**
+     * The fully qualified class name of the class that has changed.
+     * @return fully qualified class name of the class that has changed.
+     */
     public String getAffectedClass()
     {
         return affectedClass;
     }
 
+    /**
+     * Method signature of the method that has changed, if any.
+     * @return method signature or <code>null</code> if no method is affected.
+     */
     public String getAffectedMethod()
     {
         return affectedMethod;
     }
 
+    /**
+     * Field name of the field that has changed, if any.
+     * @return field name or <code>null</code> if no field is affected.
+     */
     public String getAffectedField()
     {
         return affectedField;
@@ -175,6 +192,9 @@
         return true;
     }
 
+    /**
+     * {@inheritDoc}
+     */
     public int hashCode()
     {
         int result;

[Clirr-devel] CVS: clirr/src/java/net/sf/clirr/checks AbstractClassSetChangeCheck.java,1.9,NONE AddedClassCheck.java,1.7,NONE RemovedClassCheck.java,1.8,NONE

[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-serv1342/checks

Removed Files:
	AbstractClassSetChangeCheck.java AddedClassCheck.java 
	RemovedClassCheck.java 
Log Message:
Class no longer required, as added/removed class check is now done
within the Checker class.

[Clirr-devel] CVS: clirr/src/java/net/sf/clirr/framework ClassSetChangeCheck.java,1.4,NONE

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

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

Removed Files:
	ClassSetChangeCheck.java 
Log Message:
Class no longer required, as added/removed class check is now done
within the Checker class.

[Clirr-devel] CVS: clirr/src/java/net/sf/clirr Checker.java,1.18,1.19

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

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

Modified Files:
	Checker.java 
Log Message:
Build added/removed class check into Checker class
Removed ClassSetCheck-related stuff as it is no longer used.


Index: Checker.java
===================================================================
RCS file: /cvsroot/clirr/clirr/src/java/net/sf/clirr/Checker.java,v
retrieving revision 1.18
retrieving revision 1.19
diff -u -r1.18 -r1.19
--- Checker.java	18 Jun 2004 07:57:53 -0000	1.18
+++ Checker.java	22 Jun 2004 02:56:59 -0000	1.19
@@ -32,22 +32,20 @@
 import java.net.MalformedURLException;
 import java.net.URLClassLoader;
 
-import net.sf.clirr.checks.AddedClassCheck;
 import net.sf.clirr.checks.ClassHierarchyCheck;
 import net.sf.clirr.checks.ClassScopeCheck;
 import net.sf.clirr.checks.ClassModifierCheck;
 import net.sf.clirr.checks.GenderChangeCheck;
 import net.sf.clirr.checks.InterfaceSetCheck;
-import net.sf.clirr.checks.RemovedClassCheck;
 import net.sf.clirr.checks.FieldSetCheck;
 import net.sf.clirr.checks.MethodSetCheck;
 import net.sf.clirr.event.ApiDifference;
 import net.sf.clirr.event.DiffListener;
 import net.sf.clirr.event.ScopeSelector;
+import net.sf.clirr.event.Severity;
 import net.sf.clirr.framework.ApiDiffDispatcher;
 import net.sf.clirr.framework.ClassChangeCheck;
 import net.sf.clirr.framework.ClassSelector;
-import net.sf.clirr.framework.ClassSetChangeCheck;
 import net.sf.clirr.framework.CoIterator;
 import net.sf.clirr.framework.JavaClassNameComparator;
 import net.sf.clirr.framework.CheckerException;
@@ -73,7 +71,6 @@
 
     private List listeners = new ArrayList();
 
-    private List classSetChecks = new ArrayList();
     private List classChecks = new ArrayList();
 
     private ScopeSelector scopeSelector = new ScopeSelector();
@@ -81,14 +78,6 @@
     /**
      * Package visible constructor for unit testing.
      */
-    Checker(ClassSetChangeCheck cscc)
-    {
-        classSetChecks.add(cscc);
-    }
-
-    /**
-     * Package visible constructor for unit testing.
-     */
     Checker(ClassChangeCheck ccc)
     {
         classChecks.add(ccc);
@@ -99,9 +88,6 @@
      */
     public Checker()
     {
-        classSetChecks.add(new RemovedClassCheck(this));
-        classSetChecks.add(new AddedClassCheck(this));
-
         classChecks.add(new ClassScopeCheck(this, scopeSelector));
         classChecks.add(new GenderChangeCheck(this));
         classChecks.add(new ClassModifierCheck(this));
@@ -325,11 +311,6 @@
     private void reportDiffs(ClassSet compatibilityBaseline, ClassSet currentVersion)
     {
         fireStart();
-        for (Iterator it = classSetChecks.iterator(); it.hasNext();)
-        {
-            ClassSetChangeCheck check = (ClassSetChangeCheck) it.next();
-            check.check(compatibilityBaseline, currentVersion);
-        }
         runClassChecks(compatibilityBaseline, currentVersion);
         fireStop();
     }
@@ -348,7 +329,22 @@
 
             JavaClass compatBaselineClass = (JavaClass) iter.getLeft();
             JavaClass currentClass = (JavaClass) iter.getRight();
-            if ((compatBaselineClass != null) && (currentClass != null))
+
+            if (compatBaselineClass == null)
+            {
+                final String className = compatBaselineClass.getClassName();
+                final ApiDifference diff = new ApiDifference(
+                    "Added " + className, Severity.INFO, className, null, null);
+                fireDiff(diff);
+            }
+            else if (currentClass == null)
+            {
+                final String className = currentClass.getClassName();
+                final ApiDifference diff = new ApiDifference(
+                    "Removed " + className, Severity.ERROR, className, null, null);
+                fireDiff(diff);
+            }
+            else
             {
                 // class is available in both releases
                 boolean continueTesting = true;

RE: [Clirr-devel] Report generator for Maven plugin?

[Vincent M. <vma...@pi...>]

Hi Simon,

> -----Original Message-----
> From: cli...@li... [mailto:clirr-devel-
> ad...@li...] On Behalf Of Simon Kitching
> Sent: 21 June 2004 11:37
> To: cli...@li...
> Subject: [Clirr-devel] Report generator for Maven plugin?
> 
> Hi Vincent,
> 
> I understand you are working on a Maven plugin for clirr.

Yes, it's actually sitting idle on my hard drive, waiting for my clirr
directory proposal to be accepted... ;-)

Once we are in agreement I can commit it.

> 
> Are you intending to provide an option to generate a web-page, like
the
> JDiff plugin does? If so, how are you intending to generate this web
> page?

Yes, although I have not worked on this yet.

> 
> I'm asking because I'm thinking of starting on an HTML-generating
> reporter (probably by applying an XSL stylesheet to xml) and don't
want
> to waste time if you are already working on this...

If you're interested in working on that I'd say go for it, create a XSL
stylesheet that can be used from Ant (using the <style> task for
example). I can always reuse it from within the Maven plugin.

Thanks
-Vincent

> 
> Cheers,
> 
> Simon
> 
> 
> 
> -------------------------------------------------------
> This SF.Net email is sponsored by The 2004 JavaOne(SM) Conference
> Learn from the experts at JavaOne(SM), Sun's Worldwide Java Developer
> Conference, June 28 - July 1 at the Moscone Center in San Francisco,
CA
> REGISTER AND SAVE! http://java.sun.com/javaone/sf Priority Code
NWMGYKND
> _______________________________________________
> Clirr-devel mailing list
> Cli...@li...
> https://lists.sourceforge.net/lists/listinfo/clirr-devel

RE: [Clirr-devel] Report generator for Maven plugin?

[Vincent M. <vma...@pi...>]

Hi Simon,

> -----Original Message-----
> From: cli...@li... [mailto:clirr-devel-
> ad...@li...] On Behalf Of Simon Kitching
> Sent: 21 June 2004 11:37
> To: cli...@li...
> Subject: [Clirr-devel] Report generator for Maven plugin?
> 
> Hi Vincent,
> 
> I understand you are working on a Maven plugin for clirr.

Yes, it's actually sitting idle on my hard drive, waiting for my clirr
directory proposal to be accepted... ;-)

Once we are in agreement I can commit it.

> 
> Are you intending to provide an option to generate a web-page, like
the
> JDiff plugin does? If so, how are you intending to generate this web
> page?

Yes, although I have not worked on this yet.

> 
> I'm asking because I'm thinking of starting on an HTML-generating
> reporter (probably by applying an XSL stylesheet to xml) and don't
want
> to waste time if you are already working on this...

If you're interested in working on that I'd say go for it, create a XSL
stylesheet that can be used from Ant (using the <style> task for
example). I can always reuse it from within the Maven plugin.

Thanks
-Vincent

> 
> Cheers,
> 
> Simon
> 
> 
> 
> -------------------------------------------------------
> This SF.Net email is sponsored by The 2004 JavaOne(SM) Conference
> Learn from the experts at JavaOne(SM), Sun's Worldwide Java Developer
> Conference, June 28 - July 1 at the Moscone Center in San Francisco,
CA
> REGISTER AND SAVE! http://java.sun.com/javaone/sf Priority Code
NWMGYKND
> _______________________________________________
> Clirr-devel mailing list
> Cli...@li...
> https://lists.sourceforge.net/lists/listinfo/clirr-devel

[Clirr-devel] test

[Vincent M. <vm...@pi...>]

Trying to send an email from office now that I'm back in France...

-Vincent

Re: [Clirr-devel] Removing ClassSetCheck?

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

Simon Kitching wrote:

>Hi Lars,
>
>I've been thinking about the ClassSetChangeCheck interface, and the
>classes that implement "class set checks", ie AddedClassCheck and
>RemovedClassCheck.
>
>Can you think of any other sorts of checks that could be applied to
>class sets? I must admit I can't, and it would be *very* simple to add
>the check for added/removed classes directly into the Checker class.
>
>In fact, with the CoIterator code now in the Checker.runClassChecks
>method, this is basically being done already:
>
>  if ((compatBaselineClass != null) && (currentClass != null))
>  {
>    .. apply class check
>  }
>
>If the above if-statement were changed to:
>
>  if (compatBaselineClass == null)
>  {
>    // report added class
>  }
>  else if (currentClass == null)
>  { 
>    // report removed class
>  }
>  else
>  {
>    // compare classes
>  }
>
>then a fair bit of code could be removed from clirr:
> * ClassSetChangeCheck
> * AddedClassCheck
> * RemovedClassCheck
> * AbstractClassSetChangeCheck
> + members & methods from the Checker class.
>
>
>I like the concept of "class set checks", but in the end, if there
>aren't actually any tests that take advantage of this concept, it might
>make sense to drop the idea. Of course if there *are* other "class set"
>tests that may be applied, and I just haven't spotted them, then my
>suggestion is rubbish.
>
>I hope you don't mind me proposing this....
>
>
>  
>

Of course not - removing code without removing functionality is the best 
thing that can happen.

Ultimately what I'd like to see is a unified interface for all kinds of 
checks. Problem is I couldn't come up with something that made sense 
when I started clirr.

+1 for following your suggestion until inspiration hits us. Go ahead and 
delete all that stuff...

Cheers,
Lars

Re: [Clirr-devel] Removing class from hierarchy generates "removed interface" alerts

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

Hi Simon,

consider a class "public class MyList extends AbstractList {...}". If 
you remove the extends clause completely ("public class MyList {...}"), 
Clirr should really report that MyList is not even a List any more.

It's very different from changing it to "public class MyList implements 
List {...}". Both changes are technically incompatible, but you really 
need all information if you have to decide whether such a change is 
acceptable.

-1 for removing those messages.

Cheers,
Lars

PS: Good to hear about those good results!


Simon Kitching wrote:

>Hi,
>
>I've tried running the latest clirr against commons-digester-1.5 vs
>commons-digester HEAD (because I'm familiar with the changes that ought
>to be reported). It looks pretty good, but raises one issue.
>
>When a class is removed from the inheritance hierarcy for class X, we
>also generate a bunch of alerts like:
>  Interface Y removed from class X
>  Interface Z removed from class X
>because the removed class implemented those interfaces.
>
>Should we suppress these alerts in some way, or not? It does generate
>quite a cascade of errors, but I'm not sure it is safe to simply omit
>alerts for these no-longer-implemented interfaces..
>
>
>Cheers,
>
>Simon
>
>
>  
>

[Clirr-devel] CVS: clirr/xdocs changes.xml,1.12,1.13

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

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

Modified Files:
	changes.xml 
Log Message:
* report changes in class accessability
* no error on final added to effectively-final classes


Index: changes.xml
===================================================================
RCS file: /cvsroot/clirr/clirr/xdocs/changes.xml,v
retrieving revision 1.12
retrieving revision 1.13
diff -u -r1.12 -r1.13
--- changes.xml	13 Jun 2004 11:42:54 -0000	1.12
+++ changes.xml	21 Jun 2004 10:07:10 -0000	1.13
@@ -32,6 +32,16 @@
             Removed abstract methods that are specified by an implemented interface
             are no longer reported as a compatibility problem.
         </action>
+        <action dev="s_kitching" type="add">
+            Report on classes changing accessability (top-level classes changing
+            between public and package, or nested classes changing between any
+            of public/protected/package/private).
+        </action>
+        <action dev="s_kitching" type="add">
+            It is no longer an error to add a "final" attribute to a class
+            which has no public or protected constructors, as it was always
+            impossible to derive subclasses from it anyway.
+        </action>
     </release>
 
     <release version="0.3" date="2004-05-23">

[Clirr-devel] Report generator for Maven plugin?

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

Hi Vincent,

I understand you are working on a Maven plugin for clirr.

Are you intending to provide an option to generate a web-page, like the
JDiff plugin does? If so, how are you intending to generate this web
page?

I'm asking because I'm thinking of starting on an HTML-generating
reporter (probably by applying an XSL stylesheet to xml) and don't want
to waste time if you are already working on this...

Cheers,

Simon

[Clirr-devel] Removing class from hierarchy generates "removed interface" alerts

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

Hi,

I've tried running the latest clirr against commons-digester-1.5 vs
commons-digester HEAD (because I'm familiar with the changes that ought
to be reported). It looks pretty good, but raises one issue.

When a class is removed from the inheritance hierarcy for class X, we
also generate a bunch of alerts like:
  Interface Y removed from class X
  Interface Z removed from class X
because the removed class implemented those interfaces.

Should we suppress these alerts in some way, or not? It does generate
quite a cascade of errors, but I'm not sure it is safe to simply omit
alerts for these no-longer-implemented interfaces..


Cheers,

Simon

[Clirr-devel] CVS: clirr/src/java/net/sf/clirr/checks GenderChangeCheck.java,1.9,1.10

[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-serv9365/java/net/sf/clirr/checks

Modified Files:
	GenderChangeCheck.java 
Log Message:
Aargh! Checkstyle fixes.


Index: GenderChangeCheck.java
===================================================================
RCS file: /cvsroot/clirr/clirr/src/java/net/sf/clirr/checks/GenderChangeCheck.java,v
retrieving revision 1.9
retrieving revision 1.10
diff -u -r1.9 -r1.10
--- GenderChangeCheck.java	21 Jun 2004 08:31:56 -0000	1.9
+++ GenderChangeCheck.java	21 Jun 2004 09:02:33 -0000	1.10
@@ -19,17 +19,12 @@
 
 package net.sf.clirr.checks;
 
-import java.util.ArrayList;
-import java.util.Arrays;
-import java.util.List;
-
 import net.sf.clirr.event.ApiDifference;
 import net.sf.clirr.event.Severity;
 import net.sf.clirr.framework.AbstractDiffReporter;
 import net.sf.clirr.framework.ApiDiffDispatcher;
 import net.sf.clirr.framework.ClassChangeCheck;
 import org.apache.bcel.classfile.JavaClass;
-import org.apache.bcel.util.ClassSet;
 
 /**
  * Detects gender changes (a class became an interface or vice versa).

[Clirr-devel] Removing ClassSetCheck?

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

Hi Lars,

I've been thinking about the ClassSetChangeCheck interface, and the
classes that implement "class set checks", ie AddedClassCheck and
RemovedClassCheck.

Can you think of any other sorts of checks that could be applied to
class sets? I must admit I can't, and it would be *very* simple to add
the check for added/removed classes directly into the Checker class.

In fact, with the CoIterator code now in the Checker.runClassChecks
method, this is basically being done already:

  if ((compatBaselineClass != null) && (currentClass != null))
  {
    .. apply class check
  }

If the above if-statement were changed to:

  if (compatBaselineClass == null)
  {
    // report added class
  }
  else if (currentClass == null)
  { 
    // report removed class
  }
  else
  {
    // compare classes
  }

then a fair bit of code could be removed from clirr:
 * ClassSetChangeCheck
 * AddedClassCheck
 * RemovedClassCheck
 * AbstractClassSetChangeCheck
 + members & methods from the Checker class.


I like the concept of "class set checks", but in the end, if there
aren't actually any tests that take advantage of this concept, it might
make sense to drop the idea. Of course if there *are* other "class set"
tests that may be applied, and I just haven't spotted them, then my
suggestion is rubbish.

I hope you don't mind me proposing this....



Regards,

Simon