Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed string::replace doesn't replace multiple occurrences of longer string with shorter one #200

Conversation

PiotrekB416
Copy link
Contributor

This pr fixes string::replace not replacing multiple occurrences of longer string with shorter one.
I also included unit tests from the issue

This fixes #199

api/String.cpp Outdated Show resolved Hide resolved
@per1234 per1234 added the bug label Aug 30, 2023
Co-authored-by: per1234 <accounts@perglass.com>
@per1234 per1234 dismissed their stale review August 30, 2023 21:16

Requested change has been made. Thanks!

@codecov-commenter
Copy link

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (50a27fb) 95.77% compared to head (12fb6ba) 95.77%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #200   +/-   ##
=======================================
  Coverage   95.77%   95.77%           
=======================================
  Files          13       13           
  Lines         970      970           
=======================================
  Hits          929      929           
  Misses         41       41           
Files Changed Coverage Δ
api/String.cpp 97.65% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@aentinger aentinger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent work, thank you @PiotrekB416 🚀

@@ -652,9 +652,9 @@ void String::replace(const String& find, const String& replace)
}
} else if (diff < 0) {
unsigned int size = len; // compute size needed for result
diff = 0 - diff;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, this line hurts 😉 . (Could it not be abs(diff)? However, you did not commit this crime, simply moved the line to the right position 👍 .

@aentinger aentinger merged commit 2516a69 into arduino:master Sep 4, 2023
2 checks passed
@PiotrekB416 PiotrekB416 deleted the Fix/string-replace-multiple-longer-by-shorter branch September 4, 2023 17:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

String::replace doesn't work when replacing multiple occurencies of longer String by shorter one
4 participants