[PATCH] Python-like join function for string lists
Chandler Carruth
chandlerc at gmail.com
Tue Sep 3 13:02:36 PDT 2013
================
Comment at: include/llvm/ADT/StringExtras.h:163-166
@@ -161,1 +162,6 @@
+template <typename iter>
+static inline std::string join_impl(iter Begin, iter End, StringRef Separator,
+ std::input_iterator_tag)
+{
+ std::string S;
----------------
Joerg Sonnenberger wrote:
> Shankar Kalpathi Easwaran wrote:
> > why would anyone use this version over the other ?
> The pre-computation of the output size requires that iterating twice. If the given iterator is not a forward iteratior, but e.g. a stream, it will give incorrect results silently. This was requested on IRC.
"static inline" seems contradictory. Just use "inline".
The formatting seems off. Run clang-format please.
"iter" isn't the typical typename spelling in LLVM. How about "IteratorT"?
================
Comment at: include/llvm/ADT/StringExtras.h:188-190
@@ +187,5 @@
+ size_t Len = 0;
+ for (iter I = Begin; I != End; ++I)
+ Len += (*Begin).size() + Separator.size();
+ Len -= Separator.size();
+ S.reserve(Len);
----------------
Joerg Sonnenberger wrote:
> Shankar Kalpathi Easwaran wrote:
> > We can use std::distance here
> This way it mirrors the main loop, which makes it simpler, IMO. The use of std::distance would just allow to move the Separator.size() addend out of the loop and replace it with a multiplication.
Please write it the other way: sum the segment sizes and add the correct multiple of the separator size. It seems much cleaner to me and will in some cases also be cheaper.
================
Comment at: unittests/ADT/StringRefTest.cpp:495
@@ +494,3 @@
+ bool v1_join1 = join(v1.begin(), v1.begin() + 1, ":") == join_result1;
+ ASSERT_TRUE(v1_join1);
+ bool v1_join2 = join(v1.begin(), v1.end(), ":") == join_result2;
----------------
Use EXPECT_* variants unless the subsequent tests are meaningless if this one fails.
http://llvm-reviews.chandlerc.com/D1585
More information about the llvm-commits
mailing list