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

Clean up some uses of String::substr #102427

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

AThousandShips
Copy link
Member

@AThousandShips AThousandShips commented Feb 4, 2025

Cases where the end position is either equvalent to the default or past the end of the string.

Cases:

  • X.substr(Y, X.length()) (which is always equivalent or beyond the end)
  • X.substr(Y, X.length() - Y), X.substr(Y, X.size() - (Y + 1)) (which are always equivalent)
  • X.substr(Y, X.size()) or X.substr(Y, X.size() - Y) (which is always beyond the end)
  • X.substr(Y, -1) (which is the default)

For context, X.substr(Y, -1) is equivalent to X.substr(Y, X.length() - Y), and if the second argument is greater than X.length() - Y it is clamped, so equivalent to the default again. Note that X.size() is always X.length() + 1 unless X.length() == 0 (in which case substr returns empty)

This communicates this intent more clearly, and avoids unnecessary operations, and avoids errors due to copying the code or minor changes down the line.

Didn't try to identify cases equivalent to this where the second argument was a variable.

Cases where the end position is either equvalent to the default or past
the end of the string.
Copy link
Contributor

@Ivorforce Ivorforce left a comment

Choose a reason for hiding this comment

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

I skimmed all changes; looks correct to me. I agree it's cleaner without an explicit second argument.

I would recommend waiting to merge until 4.5 due to the non zero risk of simply having gotten one of those wrong.

@AThousandShips
Copy link
Member Author

Absolutely for no sooner than 4.5, went over them with a few specific patterns (including some negations like X.substr(A + B, X.size() - A - B) and similar which were a bit harder to detect

Can split into individual categories for ease of review (i.e. the trivial case of X.substr(Y, X.length()), but shouldn't be difficult to work through the validity of them

Copy link
Member

@bruvzg bruvzg 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.

@akien-mga akien-mga modified the milestones: 4.x, 4.5 Feb 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants