From f41971f6e7f9e5ba0c590c7e35e2c1af0cd38c69 Mon Sep 17 00:00:00 2001 From: Chandler Carruth Date: Thu, 10 Sep 2015 07:51:37 +0000 Subject: [PATCH] [ADT] Fix a confusing interface spec and some annoying peculiarities with the StringRef::split method when used with a MaxSplit argument other than '-1' (which nobody really does today, but which should actually work). The spec claimed both to split up to MaxSplit times, but also to append <= MaxSplit strings to the vector. One of these doesn't make sense. Given the name "MaxSplit", let's go with it being a max over how many *splits* occur, which means the max on how many strings get appended is MaxSplit+1. I'm not actually sure the implementation correctly provided this logic either, as it used a really opaque loop structure. The implementation was also playing weird games with nullptr in the data field to try to rely on a totally opaque hidden property of the split method that returns a pair. Nasty IMO. Replace all of this with what is (IMO) simpler code that doesn't use the pair returning split method, and instead just finds each separator and appends directly. I think this is a lot easier to read, and it most definitely matches the spec. Added some tests that exercise the corner cases around StringRef() and StringRef("") that all now pass. I'll start using this in code in the next commit. git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@247249 91177308-0d34-0410-b5e6-96231b3b80d8 --- include/llvm/ADT/StringRef.h | 4 +- lib/Support/StringRef.cpp | 74 +++++++++++++++++++-------------- unittests/ADT/StringRefTest.cpp | 48 +++++++++++++++++++++ 3 files changed, 93 insertions(+), 33 deletions(-) diff --git a/include/llvm/ADT/StringRef.h b/include/llvm/ADT/StringRef.h index 414fbe4a675..34caeee801a 100644 --- a/include/llvm/ADT/StringRef.h +++ b/include/llvm/ADT/StringRef.h @@ -474,7 +474,7 @@ namespace llvm { /// Split into substrings around the occurrences of a separator string. /// /// Each substring is stored in \p A. If \p MaxSplit is >= 0, at most - /// \p MaxSplit splits are done and consequently <= \p MaxSplit + /// \p MaxSplit splits are done and consequently <= \p MaxSplit + 1 /// elements are added to A. /// If \p KeepEmpty is false, empty strings are not added to \p A. They /// still count when considering \p MaxSplit @@ -492,7 +492,7 @@ namespace llvm { /// Split into substrings around the occurrences of a separator character. /// /// Each substring is stored in \p A. If \p MaxSplit is >= 0, at most - /// \p MaxSplit splits are done and consequently <= \p MaxSplit + /// \p MaxSplit splits are done and consequently <= \p MaxSplit + 1 /// elements are added to A. /// If \p KeepEmpty is false, empty strings are not added to \p A. They /// still count when considering \p MaxSplit diff --git a/lib/Support/StringRef.cpp b/lib/Support/StringRef.cpp index f2e587cb527..88f920479dd 100644 --- a/lib/Support/StringRef.cpp +++ b/lib/Support/StringRef.cpp @@ -274,44 +274,56 @@ StringRef::size_type StringRef::find_last_not_of(StringRef Chars, } void StringRef::split(SmallVectorImpl &A, - StringRef Separators, int MaxSplit, + StringRef Separator, int MaxSplit, bool KeepEmpty) const { - StringRef rest = *this; - - // rest.data() is used to distinguish cases like "a," that splits into - // "a" + "" and "a" that splits into "a" + 0. - for (int splits = 0; - rest.data() != nullptr && (MaxSplit < 0 || splits < MaxSplit); - ++splits) { - std::pair p = rest.split(Separators); - - if (KeepEmpty || p.first.size() != 0) - A.push_back(p.first); - rest = p.second; + StringRef S = *this; + + // Count down from MaxSplit. When MaxSplit is -1, this will just split + // "forever". This doesn't support splitting more than 2^31 times + // intentionally; if we ever want that we can make MaxSplit a 64-bit integer + // but that seems unlikely to be useful. + while (MaxSplit-- != 0) { + size_t Idx = S.find(Separator); + if (Idx == npos) + break; + + // Push this split. + if (KeepEmpty || Idx > 0) + A.push_back(S.slice(0, Idx)); + + // Jump forward. + S = S.slice(Idx + Separator.size(), npos); } - // If we have a tail left, add it. - if (rest.data() != nullptr && (rest.size() != 0 || KeepEmpty)) - A.push_back(rest); + + // Push the tail. + if (KeepEmpty || !S.empty()) + A.push_back(S); } void StringRef::split(SmallVectorImpl &A, char Separator, int MaxSplit, bool KeepEmpty) const { - StringRef rest = *this; - - // rest.data() is used to distinguish cases like "a," that splits into - // "a" + "" and "a" that splits into "a" + 0. - for (int splits = 0; - rest.data() != nullptr && (MaxSplit < 0 || splits < MaxSplit); - ++splits) { - std::pair p = rest.split(Separator); - - if (KeepEmpty || p.first.size() != 0) - A.push_back(p.first); - rest = p.second; + StringRef S = *this; + + // Count down from MaxSplit. When MaxSplit is -1, this will just split + // "forever". This doesn't support splitting more than 2^31 times + // intentionally; if we ever want that we can make MaxSplit a 64-bit integer + // but that seems unlikely to be useful. + while (MaxSplit-- != 0) { + size_t Idx = S.find(Separator); + if (Idx == npos) + break; + + // Push this split. + if (KeepEmpty || Idx > 0) + A.push_back(S.slice(0, Idx)); + + // Jump forward. + S = S.slice(Idx + 1, npos); } - // If we have a tail left, add it. - if (rest.data() != nullptr && (rest.size() != 0 || KeepEmpty)) - A.push_back(rest); + + // Push the tail. + if (KeepEmpty || !S.empty()) + A.push_back(S); } //===----------------------------------------------------------------------===// diff --git a/unittests/ADT/StringRefTest.cpp b/unittests/ADT/StringRefTest.cpp index 8af07da24ea..6cf2e6a0454 100644 --- a/unittests/ADT/StringRefTest.cpp +++ b/unittests/ADT/StringRefTest.cpp @@ -230,6 +230,54 @@ TEST(StringRefTest, Split2) { expected.push_back("a"); expected.push_back("b"); expected.push_back("c"); StringRef("a,,b,c").split(parts, ',', 3, false); EXPECT_TRUE(parts == expected); + + expected.clear(); parts.clear(); + expected.push_back(""); + StringRef().split(parts, ",", 0, true); + EXPECT_TRUE(parts == expected); + + expected.clear(); parts.clear(); + expected.push_back(StringRef()); + StringRef("").split(parts, ",", 0, true); + EXPECT_TRUE(parts == expected); + + expected.clear(); parts.clear(); + StringRef("").split(parts, ",", 0, false); + EXPECT_TRUE(parts == expected); + StringRef().split(parts, ",", 0, false); + EXPECT_TRUE(parts == expected); + + expected.clear(); parts.clear(); + expected.push_back("a"); + expected.push_back(""); + expected.push_back("b"); + expected.push_back("c,d"); + StringRef("a,,b,c,d").split(parts, ",", 3, true); + EXPECT_TRUE(parts == expected); + + expected.clear(); parts.clear(); + expected.push_back(""); + StringRef().split(parts, ',', 0, true); + EXPECT_TRUE(parts == expected); + + expected.clear(); parts.clear(); + expected.push_back(StringRef()); + StringRef("").split(parts, ',', 0, true); + EXPECT_TRUE(parts == expected); + + expected.clear(); parts.clear(); + StringRef("").split(parts, ',', 0, false); + EXPECT_TRUE(parts == expected); + StringRef().split(parts, ',', 0, false); + EXPECT_TRUE(parts == expected); + + expected.clear(); parts.clear(); + expected.push_back("a"); + expected.push_back(""); + expected.push_back("b"); + expected.push_back("c,d"); + StringRef("a,,b,c,d").split(parts, ',', 3, true); + EXPECT_TRUE(parts == expected); } TEST(StringRefTest, Trim) { -- 2.34.1