<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 15 10:01:41 UTC 2012
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