[modules-dev] Review of module source code

[Rémi Forax]

this mail contains the review of Version related classe,
the review of the other classes will follow.

i've used the zip published on the openjdk page:
modules-7-ea-src-27_jun_2007.zip 
<http://www.java.net/download/openjdk/jdk7/promoted/b15/modules-7-ea-src-27_jun_2007.zip>
so sorry if the code has already changed.

package java.module:

- VersionRange.java

VersionRange#compareTo
// @Override // javac 5.0 bug,
it's not a bug of javac 5.0, @Override spec changes between 5.0 and 6.0
see http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=6399361

VersionRange#equals()
1) if (obj == null || !(obj instanceof VersionRange))
            return false;

  'null instanceof' always return false so the first test is not necessary,
  if it's for perf reason report a bug.

  so the code should be:
  if (!(obj instanceof VersionRange))
            return false;

2) if (this == obj)
        return true;
   should be done *before* instanceof test

- VersionConstraintBuilder
  constraints and normalizedConstraints should be final
  and declared as ArrayList (there are private, i.e. not visible)

  i wonder if Version and VersionRange not have an abstract package visible
  common super class.
  - you can use this super class instead of Object in your List.
  - add abstract methods like getNormalizedLowerBound() that returns
    the version itself or the versionRange lower bound
    (see #toVersionConstraint below) or
    normalizedContains(Version version) that performs an equals
    or a contains, etc.

   VersionConstraintBuilder#toVersionConstraint()
   instanceof test can be grouped:
   if (o1 instanceof Version)
     if (o2 instanceof VersionRange)
       // case 1
     else
      // case 2
  else
    if (o2 instanceof VersionRange)
       // case 3
     else
      // case 4

- VersionConstraint
 
  #equals exhibit the same glitch
 
  #toString use a StringBuffer instead of a StringBuilder

- Version
   is not a final class so methods get*Version are overridable.
  #compareTo() will not work if one getter is overriden,
  because it compare fields with getters.
 
  #equals (see above)

  #isVersion() and versionPattern can be delocalized in another class
  to allow lazy creation of the automata (this comment holds if isVersion()
  is not a method called frequently).
 
  static class VersionMatcher {
   private static  final  Pattern versionPattern;
   static {
    String regex = 
"(\\d)+(\\.(\\d)+(\\.(\\d)+(\\.(\\d)+)?)?)?(-([\\p{Alnum}-_])+)?";     
    versionPattern = Pattern.compile(regex);
    }
    static boolean isVersion(String source) {
      return versionPattern.matcher(source).matches();       
    }
  }
  public static boolean isVersion(String source) {
      return  VersionMatcher.isVersion(source);
  }

that all for today, cheers
Rémi