RFR: 7083187: Class CSS.CssValue is missing implementations of equals() and hashCode() [v15]
Alexey Ivanov
aivanov at openjdk.org
Wed Jun 7 12:48:10 UTC 2023
On Fri, 2 Jun 2023 08:12:55 GMT, Prasanta Sadhukhan <psadhukhan at openjdk.org> wrote:
>> Two CSS AttributeSet-s can be compared using the AttributeSet.isEqual() method which can fail due to missing implementation of equals method in CSS subclasses.
>> In this issue, even when two CSS AttributeSet has same 42 font size string value, Object equality fails.
>> Fixed by implementing the equality and hashCode method for CSS.FontSize class.
>>
>> All jtreg/jck tests are ok
>
> Prasanta Sadhukhan has updated the pull request incrementally with one additional commit since the last revision:
>
> Optimization removed to use local variables for non-percentage units
Overall, it covers most of the values now. There are a couple of omissions though: `BackgroundImage`, `BorderWidthValue` (it could be handled automatically by its superclass, `LengthValue`).
I assume `CssValue` is never used directly, it it?
src/java.desktop/share/classes/javax/swing/text/html/CSS.java line 2218:
> 2216: @Override
> 2217: public int hashCode() {
> 2218: return Float.hashCode(value);
Now that `equals` method takes into account `value`, `index` and `lu`, they could be added to `hashCode`.
(On the other hand, any `CssValue` object is unlikely to be used as a key in a map.)
src/java.desktop/share/classes/javax/swing/text/html/CSS.java line 2481:
> 2479: @Override
> 2480: public boolean equals(Object val) {
> 2481: return val instanceof CSS.ColorValue color && c.equals(color.c);
Is it possible that the value of `c` is null? You have the null-check in `hashCode` but you don't test it in `equals`.
`ColorValue.parseCssValue` returns `null` if it can't parse the color. So, `c` should never be `null`.
src/java.desktop/share/classes/javax/swing/text/html/CSS.java line 2545:
> 2543: @Override
> 2544: public boolean equals(Object val) {
> 2545: return val instanceof CSS.BorderStyle border && style.equals(border.style);
The `style` field is of type `CSS.Value` which doesn't implement `equals` (yet). It shouldn't be a problem, however, you may use `==` explicitly.
src/java.desktop/share/classes/javax/swing/text/html/CSS.java line 2677:
> 2675: @Override
> 2676: public int hashCode() {
> 2677: return Float.hashCode(span);
Should `hashCode` take into account `percentage` and `lu` fields which are used in `equals`?
src/java.desktop/share/classes/javax/swing/text/html/CSS.java line 2684:
> 2682: if (percentage) {
> 2683: return val instanceof CSS.LengthValue lu
> 2684: && Objects.equals(svalue, lu.svalue);
Doesn't comparing `span` work for percentage values? The comment for `span` field implies it should contain the parsed value.
This comparison could fail for the case where there's a space before the `%` in the string.
src/java.desktop/share/classes/javax/swing/text/html/CSS.java line 2918:
> 2916: hashCode |= Float.hashCode(verticalPosition);
> 2917: hashCode |= Short.hashCode(relative);
> 2918: return hashCode;
It's a nit yet you can simply return the computed value:
Suggestion:
return Float.hashCode(horizontalPosition)
| Float.hashCode(verticalPosition)
| Short.hashCode(relative);
src/java.desktop/share/classes/javax/swing/text/html/CSS.java line 3089:
> 3087: @Override
> 3088: public int hashCode() {
> 3089: return Float.hashCode(value);
Should it take into account other fields which are used in `equals`?
-------------
PR Review: https://git.openjdk.org/jdk/pull/13405#pullrequestreview-1467436669
PR Review Comment: https://git.openjdk.org/jdk/pull/13405#discussion_r1221444880
PR Review Comment: https://git.openjdk.org/jdk/pull/13405#discussion_r1221469195
PR Review Comment: https://git.openjdk.org/jdk/pull/13405#discussion_r1221476275
PR Review Comment: https://git.openjdk.org/jdk/pull/13405#discussion_r1221506112
PR Review Comment: https://git.openjdk.org/jdk/pull/13405#discussion_r1221504991
PR Review Comment: https://git.openjdk.org/jdk/pull/13405#discussion_r1221508837
PR Review Comment: https://git.openjdk.org/jdk/pull/13405#discussion_r1221511728
More information about the client-libs-dev
mailing list