2 Replies Latest reply on Feb 5, 2008 1:34 PM by adrian.brock

    VersionImpl is not correct.

      1) The compareTo() that uses a registry to do comparison
      will only work when the specific version being tested uses the registry.

      VersionImpl.compareTo(OSGiVersion) -> registry
      OSGiVersion.comapreTo(VersionImpl) does not

      The way I said to fix the problem was to pass the Comparator (really
      a registry of comparators) into the VersionRange check.

      2) The equals() doesn't work as expected.

       public boolean equals(Object object)
       {
       if (object == this)
       return true;
      
       if (object instanceof VersionImpl == false)
       return false;
      
       VersionImpl other = (VersionImpl)object;
       return (major == other.major) && (minor == other.minor) && (micro == other.micro) && qualifier.equals(other.qualifier);
       }
      


      VersionImpl ours = VersionImpl.parse("1.0.0");
      OSGiVersion osgi = OSGiVersion.parse("1.0.0");
      ours.equals(osgi) == false and vice versa

      This one is a lot harder to fix. e.g. We can't override the equals() calls
      if versions (or composites thereof) are used as keys in maps.

      Even if we fix the above code to something like:
       Version other = (Version)object;
       return registry.equals(this, other);
      


      it won't work the other way around. Also for HashMaps/Sets, etc.
      the hashCodes will likely disagree, so they are broken.

        • 1. Re: VersionImpl is not correct.
          alesj

           

          "adrian@jboss.org" wrote:
          1) The compareTo() that uses a registry to do comparison
          will only work when the specific version being tested uses the registry.

          This is expected.
          Since you need to write a comparator anyway. It's then minimal work if you also need to write a thin wrapper over existing external Version impl.
          btw: our VersionIMpl was changed, and it's now the same implementation as OSGi Version
          Except that it knows how to compare to other internal Version impls.

          Or when do you expect to have direct compareTo to external Version instance?
          Since we control all the comparison code, we/user can also introduce thin wrappers.

          If you let any Version type to be present in our VersionRange, then you would have to let version be Object, which is a lot less type safe than our Version approach with comparators and thin wrappers present.
          Not to mention that there is exactly one external Version impl present that I'm aware of.

          Or what did you have in mind with your 'pass the Comparator into the VersionRange check.' approach?
          "adrian@jboss.org" wrote:

          Also for HashMaps/Sets, etc.
          the hashCodes will likely disagree, so they are broken.

          What if we put this in Version
           public boolean equals(Object obj)
           {
           if (obj instanceof Version == false)
           return false;
          
           VersionComparatorRegistry registry = VersionComparatorRegistry.getInstance();
           return registry.compare(this, Version.class.cast(obj)) == 0;
           }
          
           public int hashCode()
           {
           return toString().hashCode();
           }
          


          • 2. Re: VersionImpl is not correct.

             

            "alesj" wrote:

            If you let any Version type to be present in our VersionRange, then you would have to let version be Object, which is a lot less type safe than our Version approach with comparators and thin wrappers present.


            You read my mind. That is the approach I'm taking. i.e. I've removed the Version
            interface altogether and just use an Object in its place in the spi.

            I originally tried Comparable but that was redundant and messy since we don't really
            use the compareTo() directly except when when comparing versions
            of the same type (and even then that's not really a requirement ;-).

            I'm also adding a default String <-> Version comparator
            so in prinicple you could just a String as your version.
            But this isn't as efficient since potentially it has to parse the String ever
            time it wants to compare it, e.g. probably twice to do a VersionRange check.

            What this does mean is that there is no VersionImpl in what is now a public spi.
            There is just our Version class that can be compared to other version classes
            (including Strings :-) using the VersionComparators.

            P.S. The hashCode() problem is unsolvable. So it is probably best to just
            document that "versions" (and anything that uses a "version" in its identity)
            should be stored in Sorted Sets and Maps using a comparator based
            on the VersionComparatorRegistry NOT the hashed collections.