<Swing Dev> [8] Review request for 8001423 javax.swing.text.StyleContext.SmallAttributeSet breaks hashcode/equals contract
Denis S. Fokin
denis.fokin at oracle.com
Wed Nov 14 18:31:42 UTC 2012
Hi Alexander,
my thoughts are below
1. I see the next scenario
// 'this' contains 3 attributes
// 'attr' does not contain attributes
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
a.) return false if the parameter is null
b.) return false if a class of the parameter differs from expected one
(in general case is not an instance of AttributeSet)
c.) return true if the objects are equal by reference
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 }
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?
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?
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