<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