[TEXT-211] TextStringBuilder.equals whatever the capacity is #281

sebx59 · 2021-11-03T10:07:39Z

Used subarrays for Arrays.equals to allow equals method to be relevant in case the 2 TextStringBuilders have not the same capacity.

Updated the test case to reflect the change

Used subarrays for Arrays.equals to allow equals method to be relevant in case the 2 TextStringBuilders have not the same capacity. Updated the test case to reflect the change

garydgregory · 2021-11-03T12:23:13Z

Hi @sebx59

What about also testing hashCode()?
The formatting looks messed up

Corrected the way hashCode is calculated, based only on the relevant characters of the buffer, independantly of the capacity. Modified also the test case with relevant test

sebx59 · 2021-11-03T13:07:03Z

You're right, my bad, new commit for hashcode
What do you mean about formatting ?

src/main/java/org/apache/commons/text/TextStringBuilder.java

garydgregory

Hi @sebx59
See 3 comments please.

garydgregory · 2021-11-03T19:42:21Z

src/main/java/org/apache/commons/text/TextStringBuilder.java

+    	if (this.size != other.size) {
+            return false;
+        }
+        return Arrays.equals(ArrayUtils.subarray(buffer, 0, size), ArrayUtils.subarray(other.buffer, 0, size));


I think we can reuse our own toString() here.

indeed, done in new commit

garydgregory · 2021-11-03T19:42:42Z

src/main/java/org/apache/commons/text/TextStringBuilder.java

@@ -1947,7 +1954,7 @@ public String getNullText() {
     */
    @Override
    public int hashCode() {
-        return Arrays.hashCode(buffer);
+        return Arrays.hashCode(ArrayUtils.subarray(buffer, 0, size));


I think we can reuse our own toString() here.

indeed, done in new commit

src/test/java/org/apache/commons/text/TextStringBuilderTest.java

kinow · 2021-11-04T21:35:30Z

src/main/java/org/apache/commons/text/TextStringBuilder.java

+        if (this.size != other.size) {
+            return false;
+        }
+        return this.toString().equals(other.toString());


@sebx59 your change makes the code look very similar to the code prior to the change that added Arrays.equals (which I also wouldn't have guessed to cause a regression here).

Maybe we could just revert it instead? 36af8b1

And also add a comment in the code so we don't accidentally add the regression again in the code. WDYT?

done ! thanks

damn tabs !

kinow

It's looking better now @sebx59 ! Thanks for adding the comments. I will take a better look later and also confirm if we can revert hashCode too (forgot to check that earlier today). We will need to squash the commits when merging it, but you can hold it until others have finished their review 👍 Great work!

garydgregory · 2022-03-28T20:08:13Z

This patch cannot be applied manually due to trailing whitespace, please fix:

git apply \temp\281.diff
/temp/281.diff:63: trailing whitespace.

/temp/281.diff:64: trailing whitespace.
        // following TEXT-211 : the hashcode of the buffer may not be equals to the hashcode of the TextStringBuilder itself
/temp/281.diff:73: trailing whitespace.

/temp/281.diff:84: trailing whitespace.
        assertEquals(hc2b2, hc3b2);
warning: 4 lines add whitespace errors.

sebx59 · 2022-03-31T08:06:05Z

should be OK now

garydgregory

Hi @sebx59
Please see pending comments.

src/main/java/org/apache/commons/text/TextStringBuilder.java

src/test/java/org/apache/commons/text/TextStringBuilderTest.java

sebx59 · 2022-03-31T11:35:49Z

new commit with requested changes
Is there a place I can see those problems before submitting th PR ?

garydgregory · 2022-03-31T12:40:51Z

In theory, the build should fail, but not all Commons components are set up properly with Checkstyle, SpotBugs, and PMD.

src/main/java/org/apache/commons/text/TextStringBuilder.java

garydgregory · 2022-03-31T14:37:11Z

https://issues.apache.org/jira/browse/TEXT-211

TEXT-211 - equals whatever the capacity is

7e0bf96

Used subarrays for Arrays.equals to allow equals method to be relevant in case the 2 TextStringBuilders have not the same capacity. Updated the test case to reflect the change

TEXT-211 - Correction to hashCode()

6fdde91

Corrected the way hashCode is calculated, based only on the relevant characters of the buffer, independantly of the capacity. Modified also the test case with relevant test

kinow reviewed Nov 3, 2021

View reviewed changes

src/main/java/org/apache/commons/text/TextStringBuilder.java Outdated Show resolved Hide resolved

garydgregory requested changes Nov 3, 2021

View reviewed changes

sebx59 added 2 commits November 4, 2021 09:18

using toString instead of subArrays

900f58e

format correction

4f13b27

kinow reviewed Nov 4, 2021

View reviewed changes

sebx59 added 2 commits November 5, 2021 11:00

rolled back to 1.8 code for equals following TEXT-211

9fe9986

format correction

2f9974e

damn tabs !

kinow reviewed Nov 5, 2021

View reviewed changes

sebx59 requested a review from garydgregory November 6, 2021 09:13

sebx59 and others added 2 commits March 31, 2022 09:59

removed trailing spaces

77a96d7

removed trailing whitespaces

b4c9f46

garydgregory requested changes Mar 31, 2022

View reviewed changes

src/main/java/org/apache/commons/text/TextStringBuilder.java Outdated Show resolved Hide resolved

src/main/java/org/apache/commons/text/TextStringBuilder.java Outdated Show resolved Hide resolved

src/test/java/org/apache/commons/text/TextStringBuilderTest.java Outdated Show resolved Hide resolved

format correction

a5dec75

garydgregory changed the title ~~TEXT-211 - TextStringBuilder.equals whatever the capacity is~~ [TEXT-211] TextStringBuilder.equals whatever the capacity is Mar 31, 2022

garydgregory reviewed Mar 31, 2022

View reviewed changes

src/main/java/org/apache/commons/text/TextStringBuilder.java Show resolved Hide resolved

garydgregory merged commit 400fb65 into apache:master Mar 31, 2022

asfgit pushed a commit that referenced this pull request Mar 31, 2022

[TEXT-211] TextStringBuilder.equals whatever the capacity is #281

fc17614

pbelousov mentioned this pull request Nov 17, 2022

Rollback TextStringBuidler#hashCode implementation to 1.8 release #387

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[TEXT-211] TextStringBuilder.equals whatever the capacity is #281

[TEXT-211] TextStringBuilder.equals whatever the capacity is #281

sebx59 commented Nov 3, 2021

garydgregory commented Nov 3, 2021

sebx59 commented Nov 3, 2021 •

edited

garydgregory left a comment

garydgregory Nov 3, 2021

sebx59 Nov 4, 2021

garydgregory Nov 3, 2021

sebx59 Nov 4, 2021

kinow Nov 4, 2021

sebx59 Nov 5, 2021

kinow left a comment •

edited

garydgregory commented Mar 28, 2022

sebx59 commented Mar 31, 2022

garydgregory left a comment

sebx59 commented Mar 31, 2022

garydgregory commented Mar 31, 2022

garydgregory commented Mar 31, 2022

[TEXT-211] TextStringBuilder.equals whatever the capacity is #281

[TEXT-211] TextStringBuilder.equals whatever the capacity is #281

Conversation

sebx59 commented Nov 3, 2021

garydgregory commented Nov 3, 2021

sebx59 commented Nov 3, 2021 • edited

garydgregory left a comment

Choose a reason for hiding this comment

garydgregory Nov 3, 2021

Choose a reason for hiding this comment

sebx59 Nov 4, 2021

Choose a reason for hiding this comment

garydgregory Nov 3, 2021

Choose a reason for hiding this comment

sebx59 Nov 4, 2021

Choose a reason for hiding this comment

kinow Nov 4, 2021

Choose a reason for hiding this comment

sebx59 Nov 5, 2021

Choose a reason for hiding this comment

kinow left a comment • edited

Choose a reason for hiding this comment

garydgregory commented Mar 28, 2022

sebx59 commented Mar 31, 2022

garydgregory left a comment

Choose a reason for hiding this comment

sebx59 commented Mar 31, 2022

garydgregory commented Mar 31, 2022

garydgregory commented Mar 31, 2022

sebx59 commented Nov 3, 2021 •

edited

kinow left a comment •

edited