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

Alexander Scherbatiy alexandr.scherbatiy at oracle.com
Thu Nov 22 15:28:41 UTC 2012


Could you review the updated fix:
    http://cr.openjdk.java.net/~alexsch/8001423/webrev.02/

- The isAttributeSetEqual method  that compares two attribute sets is 
added to the SwingUtilities2 class
     - The isDefined() method is used to know that an attribute is local
     - The attributes from the sets are compated with the equals method. 
That leads that parents are also compared by the equals method. The same 
was before the fix.
- All asymmetrically redefined equals and isEqual methods now call 
SwingUtilities2.isAttributeSetEqual() method.

Thanks,
Alexandr.

On 11/15/2012 2:01 PM, Alexander Scherbatiy wrote:
> On 11/14/2012 10:31 PM, Denis S. Fokin wrote:
>> Hi Alexander,
>>
>> my thoughts are below
>>
>> 1. I see the next scenario
>>
>>                  // 'this' contains 3 attributes
>>                  // 'attr' does not contain attributes
>
>       In this scenario the code below is unreachable because of the 
> previous check:
>
>         if (getAttributeCount() != attr.getAttributeCount()) {
>             return false;
>         }
>
>>  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
>     I have removed this check because the equals method calls 
> isEqual() method that has this check.
>     Using this check in both methods equals and isEqual is code 
> duplication.
>
>>
>> a.)  return false if the parameter is null
>      According to the implementation before the fix it should throw 
> NPE. This is a correct behavior. I do not think that it should be 
> changed.
>> b.)  return false if a class of the parameter differs from expected 
>> one (in general case is not an instance of AttributeSet)
>      Yes. This check is always implemented in the equals method.
>> c.)  return true if the objects are equal by reference
>      This is implemented in all overridden equals and isEqual method 
> (directly or from the isEqual call from equals).
>>
>> 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         }
>
>      It was decided by design that these classes should be compared 
> only by reference (so the equals method is not overridden). We do not 
> change this behavior in the current fix.
>      The current fix only makes 'equals' and 'isEqual' method 
> symmetrically for those cases there they were not symmetrically.
>
>>
>> 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?
>      There is no a code duplication because every time we check local 
> attributes from the given set with the local attributes from the 
> current set.
>      Getting local attributes from the current set depends on the set 
> class implementation. For example, the  SimpleAttributeSet contains 
> it's attributes in table and the SmallAttributeSet in an array.
>
>     So the only code duplication can be checking attribute counts. It 
> will be new public API that should be processed from the CCC request. 
> Is it really necessary?
>
>>
>> 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?
>
>       All these cases compares only local attributes.
>       The last case gets attributes from the table. The table contains 
> only local attributes.
>
>   Thanks,
>    Alexandr.
>
>>
>> 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