Page MenuHomePhabricator

[ADT] Change the behavior of `StringRef::rsplit()` when the separator is not found.
AbandonedPublic

Authored by MTC on Jun 8 2018, 10:07 PM.

Details

Summary

For StringRef::split(), it performs the search from the front to the back to find the first occurence of the separator, so when the separator is not found, returning <*this, ""> makes sense. For SrtingRef::rsplit(), it performs the search from the back to the front to find the "first" occurence of the separator too, so when the separator is not found, returning <"", *this> makes more sense.

Diff Detail

Event Timeline

MTC created this revision.Jun 8 2018, 10:07 PM
vsk added a comment.Jun 9 2018, 5:17 AM

Thanks for the patch. The StringRef change itself looks fine, but please make sure to update the callsites in-tree (e.g in llvm, clang, lldb etc).

xbolva00 accepted this revision.Jun 9 2018, 5:19 AM

Makes sense, thanks

This revision is now accepted and ready to land.Jun 9 2018, 5:19 AM
MTC added a comment.Jun 9 2018, 7:27 AM
In D47973#1127309, @vsk wrote:

Thanks for the patch. The StringRef change itself looks fine, but please make sure to update the callsites in-tree (e.g in llvm, clang, lldb etc).

Thanks for your reminder, vsk! Sorry for my negligence, I thought these corner cases were definitely included in the test case, so after the test was successful, I thought there was no potential problem.

After I re-examined some of the usages of StringRef::rsplit(), I think I need to abandon this patch.

There are three reasons for this:

  • With some usages, the string may need to be rsplit() through multiple separator. After splitting using the first separator, we can always use sample_string.rsplit(separator).first for the next split stage. However for this patch, we have to use sample_sring.rsplit(separator).first if the separator is in sample_string, and use sample_string.rsplit(separator).second if the separator is not in sample_string. The code is not clean!
  • Changing the behavior of existing libraries is dangerous especially it is difficult for me to figure out all the code logic of the StringRef::rsplit() usages.
  • Like the rsplit() in other programming languages, e.g python. rsplit() in python splits string from the right at the specified separator and returns a list of strings, and rsplit() guarantee list[0] always have values even if the separator isn't in the string. So for StringRef::rsplit(), even though StringRef::rsplit() returns a pair instead of list, we should guarantee that pair.first always has value for subsequent use.

Sorry for the lack of thinking about this patch!

What do you think, @xbolva00, @vsk!

MTC abandoned this revision.Jun 12 2018, 5:53 AM