[llvm] r282502 - Add llvm::join_items to StringExtras.

[David Blaikie via llvm-commits]

On Tue, Sep 27, 2016 at 9:46 AM Zachary Turner via llvm-commits <
llvm-commits at lists.llvm.org> wrote:

> Author: zturner
>
> Date: Tue Sep 27 11:37:30 2016
>
> New Revision: 282502
>
>
>
> URL: http://llvm.org/viewvc/llvm-project?rev=282502&view=rev
>
> Log:
>
> Add llvm::join_items to StringExtras.
>
>
>
> llvm::join_items is similar to llvm::join, which produces a string
>
> by concatenating a sequence of values together separated by a
>
> given separator.  But it differs in that the arguments to
>
> llvm::join() are same-type members of a container, whereas the
>
> arguments to llvm::join_items are arbitrary types passed into
>
> a variadic template.  The only requirement on parameters to
>
> llvm::join_items (including for the separator themselves) is
>
> that they be implicitly convertible to std::string or have
>
> an overload of std::string::operator+
>
>
>
> Differential Revision: https://reviews.llvm.org/D24880
>
>
>
> Added:
>
>     llvm/trunk/unittests/ADT/StringExtrasTest.cpp
>
> Modified:
>
>     llvm/trunk/include/llvm/ADT/StringExtras.h
>
>     llvm/trunk/unittests/ADT/CMakeLists.txt
>
>
>
> Modified: llvm/trunk/include/llvm/ADT/StringExtras.h
>
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/ADT/StringExtras.h?rev=282502&r1=282501&r2=282502&view=diff
>
>
> ==============================================================================
>
> --- llvm/trunk/include/llvm/ADT/StringExtras.h (original)
>
> +++ llvm/trunk/include/llvm/ADT/StringExtras.h Tue Sep 27 11:37:30 2016
>
> @@ -155,6 +155,8 @@ static inline StringRef getOrdinalSuffix
>
>  /// it if it is not printable or if it is an escape char.
>
>  void PrintEscapedString(StringRef Name, raw_ostream &Out);
>
>
>
> +namespace detail {
>
> +
>
>  template <typename IteratorT>
>
>  inline std::string join_impl(IteratorT Begin, IteratorT End,
>
>                               StringRef Separator,
> std::input_iterator_tag) {
>
> @@ -189,12 +191,64 @@ inline std::string join_impl(IteratorT B
>
>    return S;
>
>  }
>
>
>
> +template <typename Sep>
>
> +inline void join_items_impl(std::string &Result, Sep Separator) {}
>
> +
>
> +template <typename Sep, typename Arg>
>
> +inline void join_items_impl(std::string &Result, Sep Separator,
>
> +                            const Arg &Item) {
>
> +  Result += Item;
>
> +}
>
> +
>
> +template <typename Sep, typename Arg1, typename... Args>
>
> +inline void join_items_impl(std::string &Result, Sep Separator, const
> Arg1 &A1,
>
> +                            Args &&... Items) {
>
> +  Result += A1;
>
> +  Result += Separator;
>
> +  join_items_impl(Result, Separator, std::forward<Args>(Items)...);
>
> +}
>
> +
>
> +inline size_t join_one_item_size(char C) { return 1; }
>
> +inline size_t join_one_item_size(const char *S) { return S ? ::strlen(S)
> : 0; }
>
> +
>
> +template <typename T> inline size_t join_one_item_size(const T &Str) {
>
> +  return Str.size();
>
> +}
>
> +
>
> +inline size_t join_items_size() { return 0; }
>
> +
>
> +template <typename A1> inline size_t join_items_size(const A1 &A) {
>
> +  return join_one_item_size(A);
>
> +}
>
> +template <typename A1, typename... Args>
>
> +inline size_t join_items_size(const A1 &A, Args &&... Items) {
>
> +  return join_one_item_size(A) +
> join_items_size(std::forward<Args>(Items)...);
>
> +}
>
> +}
>
> +
>
>  /// Joins the strings in the range [Begin, End), adding Separator between
>
>  /// the elements.
>
>  template <typename IteratorT>
>
>  inline std::string join(IteratorT Begin, IteratorT End, StringRef
> Separator) {
>
>    typedef typename std::iterator_traits<IteratorT>::iterator_category tag;
>
> -  return join_impl(Begin, End, Separator, tag());
>
> +  return detail::join_impl(Begin, End, Separator, tag());
>
> +}
>
> +
>
> +/// Joins the strings in the parameter pack \p Items, adding \p Separator
>
> +/// between the elements.  All arguments must be implicitly convertible to
>
> +/// std::string, or there should be an overload of
> std::string::operator+=()
>
> +/// that accepts the argument explicitly.
>
> +template <typename Sep, typename... Args>
>
> +inline std::string join_items(Sep Separator, Args &&... Items) {
>
> +  std::string Result;
>
> +  if (sizeof...(Items) == 0)
>
> +    return Result;
>
> +
>
> +  size_t NS = detail::join_one_item_size(Separator);
>
> +  size_t NI = detail::join_items_size(std::forward<Args>(Items)...);
>
> +  Result.reserve(NI + (sizeof...(Items) - 1) * NS + 1);
>
> +  detail::join_items_impl(Result, Separator,
> std::forward<Args>(Items)...);
>
> +  return Result;
>
>  }
>
>
>
>  } // End llvm namespace
>
>
>
> Modified: llvm/trunk/unittests/ADT/CMakeLists.txt
>
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/ADT/CMakeLists.txt?rev=282502&r1=282501&r2=282502&view=diff
>
>
> ==============================================================================
>
> --- llvm/trunk/unittests/ADT/CMakeLists.txt (original)
>
> +++ llvm/trunk/unittests/ADT/CMakeLists.txt Tue Sep 27 11:37:30 2016
>
> @@ -53,6 +53,7 @@ set(ADTSources
>
>    SparseBitVectorTest.cpp
>
>    SparseMultiSetTest.cpp
>
>    SparseSetTest.cpp
>
> +  StringExtrasTest.cpp
>
>    StringMapTest.cpp
>
>    StringRefTest.cpp
>
>    TinyPtrVectorTest.cpp
>
>
>
> Added: llvm/trunk/unittests/ADT/StringExtrasTest.cpp
>
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/ADT/StringExtrasTest.cpp?rev=282502&view=auto
>
>
> ==============================================================================
>
> --- llvm/trunk/unittests/ADT/StringExtrasTest.cpp (added)
>
> +++ llvm/trunk/unittests/ADT/StringExtrasTest.cpp Tue Sep 27 11:37:30 2016
>
> @@ -0,0 +1,52 @@
>
> +//===- StringExtrasTest.cpp - Unit tests for String extras
> ----------------===//
>
> +//
>
> +//                     The LLVM Compiler Infrastructure
>
> +//
>
> +// This file is distributed under the University of Illinois Open Source
>
> +// License. See LICENSE.TXT for details.
>
> +//
>
>
> +//===----------------------------------------------------------------------===//
>
> +
>
> +#include "llvm/ADT/StringExtras.h"
>
> +#include "gtest/gtest.h"
>
> +
>
> +using namespace llvm;
>
> +
>
> +TEST(StringExtrasTest, Join) {
>
> +  std::vector<std::string> Items;
>
> +  EXPECT_EQ("", join(Items.begin(), Items.end(), " <sep> "));
>
> +
>
> +  Items = {"foo"};
>
> +  EXPECT_EQ("foo", join(Items.begin(), Items.end(), " <sep> "));
>
> +
>
> +  Items = {"foo", "bar"};
>
> +  EXPECT_EQ("foo <sep> bar", join(Items.begin(), Items.end(), " <sep> "));
>
> +
>
> +  Items = {"foo", "bar", "baz"};
>
> +  EXPECT_EQ("foo <sep> bar <sep> baz",
>
> +            join(Items.begin(), Items.end(), " <sep> "));
>
> +}
>
> +
>
> +TEST(StringExtrasTest, JoinItems) {
>
> +  const char *Foo = "foo";
>
> +  std::string Bar = "bar";
>
> +  llvm::StringRef Baz = "baz";
>
> +  char X = 'x';
>
> +
>
> +  EXPECT_EQ("", join_items(" <sep> "));
>
> +  EXPECT_EQ("", join_items('/'));
>
> +
>
> +  EXPECT_EQ("foo", join_items(" <sep> ", Foo));
>
> +  EXPECT_EQ("foo", join_items('/', Foo));
>
> +
>
> +  EXPECT_EQ("foo <sep> bar", join_items(" <sep> ", Foo, Bar));
>
> +  EXPECT_EQ("foo/bar", join_items('/', Foo, Bar));
>
> +
>
> +  EXPECT_EQ("foo <sep> bar <sep> baz", join_items(" <sep> ", Foo, Bar,
> Baz));
>
> +  EXPECT_EQ("foo/bar/baz", join_items('/', Foo, Bar, Baz));
>
> +
>
> +  EXPECT_EQ("foo <sep> bar <sep> baz <sep> x",
>
> +            join_items(" <sep> ", Foo, Bar, Baz, X));
>
> +
>
> +  EXPECT_EQ("foo/bar/baz/x", join_items('/', Foo, Bar, Baz, X));
>

FWIW I wouldn't feel the need to test each of these 8 cases.

For example, it looks like the ability for join_items to accept different
kinds of separators is orthogonal to how it accepts the things to join. So
I'd probably only test one instance of each of the separators (" <sep> "
and '/') in a case where they appear at all, then just test the other cases
with a single separator chosen semi-randomly (probably a short string, like
'/' or ',', though, to make the tests simpler)

Ever the 4 cases of {{Foo}, {Foo, Bar}, {Foo, Bar, Baz}, {Foo, Bar, Baz, X}
seem a bit verbose. What does each cover that the others don't? Perhaps
just testing the empty case, the single item case, and {Foo, Bar, Baz, X} ?
(skip the {Foo, Bar}, and {Foo, Bar, Baz} cases?) You could maybe even test
{foo} as the single case and {Bar, Baz, X} in the other.

Could even decide that testing all string-able things isn't necessary (it's
not a closed set anyway) & skip some of these. Or write a custom streamable
type that has no other features and assume that if that works, everything
else does (because we already know they're streamable - their docs say they
are, etc, we don't need to test that they are)

Just some ideas, in case any sound good to you,

- Dave


>
> +}
>
>
>
>
>
> _______________________________________________
>
> llvm-commits mailing list
>
> llvm-commits at lists.llvm.org
>
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20161010/f020542e/attachment.html>