Skip to content

Fix bug in replace_all#69

Merged
WhyPenguins merged 2 commits intothoth-tech:t2-2024from
matt439:fix-replace-all
Sep 30, 2024
Merged

Fix bug in replace_all#69
WhyPenguins merged 2 commits intothoth-tech:t2-2024from
matt439:fix-replace-all

Conversation

@matt439
Copy link

@matt439 matt439 commented Sep 2, 2024

Description

While creating unit tests, I identified a bug in the replace_all function which causes an infinite loop if the substr argument is an empty string. I added a check at the beginning of the function which detects if the sub-string is empty. If so, the input string text is returned.

There are multiple ways to handle this edge case. One is to throw an exception, another is to return an empty string. I believe the most logical response it to return the unaltered input string as it shows to the user that no action will be taken with an empty sub-string.

This is not a breaking change as the current behavior always results in a program crash.

Type of change

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

I added an extra line of testing to test_text.cpp which uses an empty string as the substr argument. It runs as expected. In a separate pull request, I have created unit testing for replace_all which also tests this function's edge cases.

Testing Checklist

  • Tested with sktest

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • My changes generate no new warnings

Copy link

@Liquidscroll Liquidscroll left a comment

Choose a reason for hiding this comment

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

This looks good to me! I think it makes sense to return the entire input text when the substring is empty, I think it's pretty logical.

Copy link

@DarrenSunandar DarrenSunandar left a comment

Choose a reason for hiding this comment

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

This pull request is well-structured and logical change made. The early check for an empty substring ensures that the function won't crash. Also, nice to ensures that when the substring is empty, the original string is returned, preventing an infinite loop.

Copy link

@WhyPenguins WhyPenguins left a comment

Choose a reason for hiding this comment

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

Looks good!

@WhyPenguins WhyPenguins changed the base branch from develop to t2-2024 September 30, 2024 19:32
@WhyPenguins WhyPenguins merged commit d0e94c6 into thoth-tech:t2-2024 Sep 30, 2024
@matt439 matt439 deleted the fix-replace-all branch November 1, 2024 01:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants