[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