<Swing Dev> [8] Review request for 8001423 javax.swing.text.StyleContext.SmallAttributeSet breaks hashcode/equals contract
Sergey Bylokhov
Sergey.Bylokhov at oracle.com
Tue Dec 4 11:40:57 UTC 2012
Hi, Alexander.
Looks like this code in StyleContext:
903 if (attr instanceof SmallAttributeSet) {
904 return attr == this;
905 }
is unnecessary since you add the same chech in:
SwingUtilities2.isAttributeSetEqual
22.11.2012 19:28, Alexander Scherbatiy wrote:
>
> 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.
Is it possible to add some comments to the method about this?
Thanks.
> - 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.
>>>>>>
>>>>>
>>>>
>>>>
>>>
>>
>
>
--
Best regards, Sergey.
More information about the swing-dev
mailing list