<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 14:19:49 UTC 2012
Hi, Alexander.
Thanks for clarification. I suggest check equal/isEqual contract in the
separate CR?
Fix looks good.
04.12.2012 17:59, Alexander Scherbatiy wrote:
> On 12/4/2012 3:40 PM, Sergey Bylokhov wrote:
>> Hi, Alexander.
>> Looks like this code in StyleContext:
>>
>> 903 if (attr instanceof SmallAttributeSet) {
>> 904 return attr == this;
>> 905 }
>>
> This check is preserved because of the backward compatibility (the
> fix does not change isEqual() and equals() methods logic).
> It says that two instances of the SmallAttributeSet class are equal
> only by reference, not by elements.
>
> The check from the SwingUtilities2.isAttributeSetEqual method is
> different and used for a performance reason (comparing a set with
> itself always returns true).
>
> Thanks,
> Alexandr.
>
>> 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