Common: Fix a potential infinite loop in ReplaceAll

[lioncash]

Prior to this change, it was possible to cause an infinite loop by making the string to be replaced and the replacing string the same thing. Came across this while writing the string unit-tests.

e.g.
std::string some_str = "test";
ReplaceAll(some_str, "test", "test");

This also changes the replacing in a way that doesn't require starting from the beginning of the string on each replacement iteration.

[delroth]

Tests please :)

[BhaaLseN]

Does this successfully replace all occurrences? The += dest.length() looks like it might not do that.

[shuffle2]

assignment in conditionals are generally gross. maybe just strcmp()(stl equiv) before?

[magcius]

This will change behavior for ReplaceAll("tetestst", "test", "");, but I hope that the new behavior is what's expected.

[lioncash]

@delroth Sure, I'll add a basic test when I get home.

@BhaaLseN Example: http://ideone.com/07X7CC

@shuffle2 strcmp's C++ equivalent is std::string's == operator, so I don't see how that would work (and not be messier).

@magcius Yeah, that's expected behavior. It should only be used to process the given string as is without modifying the already replaced portions of it during the function's execution.

[BhaaLseN]

If its expected, it should be documented somewhere.

Tho, I don't know how its used in the Dolphin codebase, but I usually have the use-case of normalizing multiple spaces into a single space when I call a method called "ReplaceAll". With the current implementation, I'd still end up with multiple spaces for something like ReplaceAll("..something...with...many.spaces..", "..", ".") (space replaced with dots, as GH seems to strip them).

If we don't want/need that here (or don't care about that case), its fine by me.

[lioncash]

@BhaaLseN I think I'm misunderstanding what you're saying or something. From my POV, ReplaceAll should usually mean "Replace, in the given string, all occurrences of [target] with [replacement]" not "Replace, in the given string and replacements in the string, ...".

In regards to ReplaceAll("..something...with...many.spaces..", "..", "."), why would you not expect the result: .something..with..many.spaces.? You are telling it to replace all occurrences of .. with .

Even C# does this behavior in its replacing method for example: http://ideone.com/ZPRqlz

[shuffle2]

Yea, I haven't used a replace() which is modifying in-place and recursive...

[BhaaLseN]

Exactly, but the C# method is called Replace; not ReplaceAll (which might be odd, considering that Replace replaces all occurrences, but not those that have already been replaced - which is what my usual C# ReplaceAll methods do).

But with that cleared up, fine by me as it is.

[shuffle2]

strcmp's C++ equivalent is std::string's == operator, so I don't see how that would work (and not be messier).

Oops, I totally missed that line of the patch, nvm. lgtm.