<Swing Dev> [8] Review request for 8001423 javax.swing.text.StyleContext.SmallAttributeSet breaks hashcode/equals contract

Denis S. Fokin denis.fokin at oracle.com
Wed Nov 14 18:31:42 UTC 2012


Hi Alexander,

my thoughts are below

1. I see the next scenario

                  // 'this' contains 3 attributes
                  // 'attr' does not contain attributes

  925             boolean result = true;
  926
  927             Enumeration names = attr.getAttributeNames();

                  // 'names' is an empty enumeration
                  // by this reason we do not get into the loop

  928             while (result && names.hasMoreElements()) {
  929                 Object name = names.nextElement();
  930                 result = 
attr.getAttribute(name).equals(getLocalAttribute(name));
  931             }

                  // three attributes is not the same as no attributes
                  // but we have not changed the default value of the
                  // result variable, so return true.

  932
  933             return result;

2. You have removed the next code form the SimpleAttributeSet.java file

  305         if (this == obj) {
  306             return true;
  307         }

I suggest the next checks in the beginning of each equals method

a.)  return false if the parameter is null
b.)  return false if a class of the parameter differs from expected one 
(in general case is not an instance of AttributeSet)
c.)  return true if the objects are equal by reference

4.

By contrast with StyleContext.SmallAttributeSet.equals() we do not 
change anything in StyleContext.NamedStyle.equals()

StyleContext.java
1431         /**
1432          * Checks whether two attribute sets are equal.
1433          *
1434          * @param attr the attribute set to check against
1435          * @return true if the same
1436          * @see AttributeSet#isEqual
1437          */
1438         public boolean isEqual(AttributeSet attr) {
1439             return attributes.isEqual(attr);
1440         }

5.
In you current implementation we have a code duplication. This code is 
common for StyleContext.SmallAttributeSet, SimpleAttributeSet, 
MuxingAttributeSet (and may be for future NamedStyle).

  921             if (getAttributeCount() != attr.getAttributeCount()) {
  922                 return false;
  923             }
  924
  925             boolean result = true;
  926
  927             Enumeration names = attr.getAttributeNames();
  928             while (result && names.hasMoreElements()) {
  929                 Object name = names.nextElement();
  930                 result = 
attr.getAttribute(name).equals(getLocalAttribute(name));
  931             }
  932
  933             return result;

May be we should somehow reuse our code here?

6.

In the next places we have different approaches for the comparison of 
attributes.

- comparation of local attributes (StyleContext)
- comparation of local attributes 'Approach #2' (MuxingAttributeSet)
- comparation of attributes

Is it intentional or it should be implemented the same way?

StyleContext.java

  920         private boolean isAttributeSetEqual(AttributeSet attr) {
  921             if (getAttributeCount() != attr.getAttributeCount()) {
  922                 return false;
  923             }
  924
  925             boolean result = true;
  926
  927             Enumeration names = attr.getAttributeNames();
  928             while (result && names.hasMoreElements()) {
  929                 Object name = names.nextElement();
  930                 result = 
attr.getAttribute(name).equals(getLocalAttribute(name));
  931             }
  932
  933             return result;
  934         }

  795         Object getLocalAttribute(Object nm) {
  796             if (nm == StyleConstants.ResolveAttribute) {
  797                 return resolveParent;
  798             }
  799             Object[] tbl = attributes;
  800             for (int i = 0; i < tbl.length; i += 2) {
  801                 if (nm.equals(tbl[i])) {
  802                     return tbl[i+1];
  803                 }
  804             }
  805             return null;
  806         }


MuxingAttributeSet.java

  167     public boolean isEqual(AttributeSet attr) {
  168         if (this == attr) {
  169             return true;
  170         }
  171
  172         if (getAttributeCount() != attr.getAttributeCount()) {
  173             return false;
  174         }
  175
  176         boolean result = true;
  177
  178         Enumeration names = attr.getAttributeNames();
  179         while (result && names.hasMoreElements()) {
  180             Object name = names.nextElement();
  181             Object localAttribute = getLocalAttribute(name);
  182             result = localAttribute != null && 
attr.getAttribute(name).equals(localAttribute);
  183         }
  184
  185         return result;
  186     }



  188     private Object getLocalAttribute(Object name) {
  189         for (AttributeSet as : getAttributes()) {
  190             Enumeration<?> names = as.getAttributeNames();
  191             while (names.hasMoreElements()) {
  192                 Object attributeName = names.nextElement();
  193                 if (attributeName.equals(name)) {
  194                     return as.getAttribute(name);
  195                 }
  196             }
  197         }
  198         return null;
  199     }

SimpleAttributeSet.java

  115     public boolean isEqual(AttributeSet attr) {
  116         if (this == attr) {
  117             return true;
  118         }
  119
  120         if (getAttributeCount() != attr.getAttributeCount()) {
  121             return false;
  122         }
  123
  124         boolean result = true;
  125
  126         Enumeration names = attr.getAttributeNames();
  127         while (result && names.hasMoreElements()) {
  128             Object name = names.nextElement();
  129             result = attr.getAttribute(name).equals(table.get(name));
  130         }
  131
  132         return result;
  133     }

Thank you,
    Denis.

On 11/12/2012 5:31 PM, Alexander Scherbatiy wrote:
>
> Could you review the updated fix:
> http://cr.openjdk.java.net/~alexsch/8001423/webrev.01/
>
> See my comments below:
>
> On 11/6/2012 6:26 PM, Denis S. Fokin wrote:
>> Hi Alexander,
>>
>> I found several places that could be improved, in my opinion.
>>
>> SimpleAttributeSet.java
>> 120 boolean result = true;
>>
>> I would not assign 'true' as a default value. If the 'names'
>> enumeration does not have any elements, we return true even not trying
>> to compare anything.
> We always check before that both attribute sets have the same count. So
> if the second attribute set is empty that means that the first set also
> empty.
>
>>
>> // Does not have elements, so returns true.
>> 123 while (result && names.hasMoreElements()) {
> That is ok. Comparing two empty attribute sets should return true
> because they are equal to each other.
>>
>>
>> MuxingAttributeSet.java
>> 130 for (int i = 0; i < as.length; i++) {
>> Why we do not use foreach loop here?
> Fixed.
>>
>>
>> AbstractDocument.java
>> 1874 && !(obj instanceof AbstractElement)) {
>> I am not an expert in Swing, so this check is not quite clear to me.
>> Why we check AbstractDocument.AbstractElement and do not check, for
>> instance, SimpleAttributeSet? Is it only because of "the old
>> AbstractElement behaviour"?
> There is a theory how to implement equals/hashCode methods correctly in
> Java.
> For example, If there is a class Point{ double x; double y; } the equals
> method can be:
> --------------------------------------------
> public boolean equals(Object obj) {
> if (obj instanceof Point) {
> Point point = (Point) obj;
> return x == point.x && y == point.y;
> }
> return false;
> }
> --------------------------------------------
>
> And there is a child class
> --------------------------------------------
> class ColorPoint extends Point{
> Color color;
> }
> --------------------------------------------
>
> Point point = new Point(3,4);
> ColorPoint c = new ColorPoint(3, 4, Color.GREEN);
> ColorPoint bluePoint = new ColorPoint(3, 4, Color.BLUE);
>
> What we want that the greenPoint was different from the bluePoint.
> And we know that the check point.equals(greenPoint) returns true because
> Point 'equals' method checks only coordinates.
>
> So the ColorPoint equals method could look like:
> -----------------------------------------------
> public boolean equals(Object obj) {
> if (obj instanceof ColorPoint) {
> return (((ColorPoint) obj).color == this.color) && super.equals(obj);
> } else if (obj instanceof Point) {
> return super.equals(obj);
> }
> return false;
> }
> -----------------------------------------------
> The idea is that a child class should add an additional check for
> objects that have the same instance.
>
> The check 'if ((obj instanceof AttributeSet) && !(obj instanceof
> AbstractElement))' in the AbstractDocument has been added for the same
> reason.
> If an object is an instance of AbstractDocument we return just (this ==
> obj) in other case if the object is and instance of AttributeSet we
> return isEqual((AttributeSet) obj).
>
> The theory also says that it is better to add 'isEqual' rather to
> override the 'equals' method because in the second case
> the hashCode method should be overriden accordingly and for mutable
> object it could have troubles.
>
>
> I decided to changed the fix to:
> - make the equals and isEqual method symmetrically
> - updated the hashCode method only if they were implemented
>
> And leave all others as is to not break the backward compatibility.
>
>> By the way, I would say that we should check for equality by reference
>> in the first place to get rid of comparing attributes if we call
>> equals method for the same object.
>
> Fixed.
>
> Thanks,
> Alexandr.
>>
>> Thank you,
>> Denis.
>>
>>
>> On 11/1/2012 5:30 PM, Alexander Scherbatiy wrote:
>>>
>>> Hello,
>>>
>>> Could you review the fix:
>>> bug: http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8001423
>>> webrev: http://cr.openjdk.java.net/~alexsch/8001423/webrev.00
>>>
>>> The 'equals' method from the SmallAttributeSet class compares local
>>> attributes from the current set with all attributes (local and parent)
>>> from the second set if local counts are equal.
>>> So if we check the following text attributes:
>>> attr1: [ elem1 = value1] // getAttributeCount = 1
>>> attr2: [ Parent = [ elem1 = value1] ] // getAttributeCount = 1
>>>
>>> The results are:
>>> attr1.equals(attr2) = false // attr1 does not contain element with name
>>> Parent
>>> attr2.equals(attr1) = true // attr2 has element elem1 in the parent set
>>>
>>> All other classes that implement AttributeSet interface has the same
>>> problems with the hashcode/equals contract.
>>>
>>> The fix compares all local attributes from one set with all local
>>> attributes from the another set in the equals method.
>>> If both sets have parents, they will be compared automatically (in the
>>> same way as it was before).
>>>
>>>
>>> SimpleAttributeSet: Hashtable returns hashcode based on both keys and
>>> values. The fix redefines it to return a hash code only for values.
>>> MuxingAttributeSet: There is no direct way to get all local attributes.
>>> The getLocalAttribute method returns a value only if name is local.
>>> AbstractElement: If both attribute sets are AbstractElement they are
>>> compared by reference. In other case they are compared by checking local
>>> attributes. This preserves the old AbstractElement behaviour.
>>>
>>> The javax/swing/text/AttributeSet/8001423/bug8001423.java test is added.
>>>
>>> I run the javax/swing/text tests and there is no new failures after the
>>> fix.
>>>
>>> Thanks,
>>> Alexandr.
>>>
>>
>
>




More information about the swing-dev mailing list