7
\$\begingroup\$

Trying something out of my comfort zone here, and attempting to implement a D-style range around std::string_view.

Looking for any and all suggestions or improvements.

One of the things I'm not exactly happy about is having to declare difference_type, pointer and reference just to fullfill the forward_iterator concept properly, yet not being able to actually use them.

I feel that this allowing me to initialize a vector straight from the iterator pair is worth the clunkiness, but I welcome any opinion on the subject, or suggestions on how to make it better.

Thanks!

#include <iterator>
#include <string_view>

template <typename CharT>
struct SplitStringIterator {
  using difference_type = std::ptrdiff_t;
  using value_type = std::basic_string_view<CharT>;
  using pointer = std::basic_string_view<CharT>*;
  using reference = std::basic_string_view<CharT>&;
  using iterator_category = std::forward_iterator_tag;

  SplitStringIterator(value_type str, CharT delim)
      : remainder_(str), delim_(delim) {
    advance_();
  }

  SplitStringIterator& operator++() {
    advance_();
    return *this;
  }

  SplitStringIterator operator++(int) {
    SplitStringIterator other = *this;
    advance_();
    return other;
  }

  value_type operator*() const { return sub_str_; }

  bool operator==(SplitStringIterator const& rhs) const {
    return sub_str_.data() == rhs.sub_str_.data() &&
           remainder_.data() == rhs.remainder_.data() && delim_ == delim_;
  }

  bool operator!=(SplitStringIterator const& rhs) const {
    return !(*this == rhs);
  }

 private:
  value_type sub_str_;
  value_type remainder_;
  const CharT delim_;

  void advance_() {
    auto delim_pos = remainder_.find(delim_);

    if (delim_pos == value_type::npos) {
      sub_str_ = remainder_;
      remainder_ = value_type();
    } else {
      sub_str_ = value_type(&*remainder_.begin(), delim_pos);
      remainder_.remove_prefix(delim_pos + 1);
    }
  }
};

template <typename CharT>
struct SplitString {
  SplitString(std::basic_string_view<CharT> src, CharT d)
      : source(src), delim(d) {}

  std::basic_string_view<CharT> source;
  CharT delim;

  SplitStringIterator<CharT> begin() const {
    return SplitStringIterator<CharT>(source, delim);
  }

  SplitStringIterator<CharT> end() const {
    return SplitStringIterator<CharT>(std::basic_string_view<CharT>(), delim);
  }
};

// Returns a lazy string splitter.
template <typename CharT>
SplitString<CharT> split(std::basic_string_view<CharT> str, CharT delim) {
  return SplitString<CharT>(str, delim);
}

Usage example:

#include <vector>
#include <iostream>

void foo() {
  std::string data = "aa:bb:cccc";
  auto parts = split(std::string_view(data), ':');

  for(auto p : parts) {
    std::cout << p << std::endl;
  }

  //if we need random access:
  std::vector<std::string_view> parts_indexable(parts.begin(), parts.end());
}
\$\endgroup\$

2 Answers 2

6
\$\begingroup\$

Implementation

  • operator==(SplitStringIterator const&) only compares pointers for equality, not lengths. This might not be intended.
  • The ForwardIterator concept isn't actually fulfilled: For that, operator* has to return a reference! (And it makes sense, too: You cannot provide a multipass guarantee in case the contents get modified, i.e. a previously different CharT value gets changed to delim.)
  • Why restrict delim to CharT, when it could just as easily be a std::basic_string_view<CharT>? The advance_() logic would have to be updated for the varying length, but that's the only non-type change I can see that would be necessary. (Though one could add a conversion or a special case for single CharT delimiters.)

Small caveat: If parts_indexable had a longer lifetime than data (e.g. it got returned by foo), all its contents would dangle (though this is inherent problem of std::string_view, and not of your implementation). This could be fixed by making explicit copies by changing the type of parts_indexable to std::vector<std::string>. (Not a criticism of your work, just a note for users of this iterator).

\$\endgroup\$
6
  • \$\begingroup\$ Thanks for your reply. 1. I figured that's a non issue because two iterators in the same string with the same delimiter should be equivalent in that context 2. Yeah, you caught me, thankfully, it's still enough to fool std::vector. I figure that returning a reference to sub_str_ would be a lot worse since assigning stuff to it would be altering the iterator and not the underlying data. Maybe making reference a simple string_view<> instead of string_view<>& would do the trick. \$\endgroup\$
    – user128454
    Nov 18, 2017 at 6:08
  • \$\begingroup\$ 3. That's the plan, I just wanted to sanity check what I have first before proceeding with a more complex variant. \$\endgroup\$
    – user128454
    Nov 18, 2017 at 6:08
  • 1
    \$\begingroup\$ @Frank: Regarding 1. They are only equivalent if their lengths are. auto a = "Hello World!"s; auto b = std::string_view(a); auto c = b.substr(0, 5); Now both pointers (b.data() and c.data()) are the pointing to the same locations yet they clearly belong to different std::string_views. Regarding 2: Just make it an input iterator (as you cannot provide the multipass guarantee anyways). This also prevents dangling references, as they are allowed to return non-references from operator*()! \$\endgroup\$
    – hoffmale
    Nov 18, 2017 at 6:21
  • \$\begingroup\$ What I'm getting at is that due to the logic of the class, two iterators starting at the same location with the same delimiter will necessarily have their length set to the same value. Thanks for the pointer on input iterators, that's probably the best way to tackle that indeed. Though I could get around the multipass guarantee by simply documenting that altering the string invalidates the iterators. \$\endgroup\$
    – user128454
    Nov 18, 2017 at 6:28
  • \$\begingroup\$ @Frank: Continuing the example from above: auto it1 = split(b, 'l').begin(); auto it2 = split(c, 'l').begin(); They start at the same location, but still have different lengths! \$\endgroup\$
    – hoffmale
    Nov 18, 2017 at 6:30
2
\$\begingroup\$

Naming

My preference for range would be string_splitter. For iterator, I would prefer splitter_iterator.

Semantics

The semantics look much like std::istream_iterator. As a result, since std::string_view is non-owning range and charT is usually default constructible, may be it is good to put a default constructor, so iterator based algorithms could still use it.

range_splitter():
         //value initialize everything to zero/default
{}

Default-constructed iterator would act like a sentinel value for the range.

Also, it seems that providing a reference on dereference is quite risky, since users could insert delim somewhere in the middle of the range they got. The idea is good, but I believe it could be improved to make it harder to use incorrectly. InputIterator seems much more natural for me.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.