Iterating a vector to second to last element with index vs iterator

Question

When iterating from the beginning of a C++11 std::vector to the second to last element, what would be the preferred style?

std::vector<const char*> argv;
std::string str;

Should this kind of more C++-esque method be used

for (const auto& s: decltype(argv)(argv.begin(), argv.end()-1)) {
    str += std::string(s) + ' ';
}

or should the more traditional way be preferred?

for (size_t i = 0; i < argv.size() - 1; ++i) {
    str += std::string(argv[i]);
}

Answer 1

Please don't write this:

for (const auto& s: decltype(argv)(argv.begin(), argv.end()-1)) {

First and foremost, nobody (including you) will understand this when you look back on it. Secondly, since decltype(argv) is a vector, this is copying a whole bunch of elements ... all simply because you want to avoid iterating one of them? That's very wasteful.

It also has another problem, which is shared by your second option.

This:

for (size_t i = 0; i < argv.size() - 1; ++i) {

is much more subtly problematic, because size() is unsigned. So if argv happens to be empty, argv.size() - 1 is going to be some enormously large number, and you're actually going to access all these invalid elements of the array leading to undefined behavior.

For iterators, if argv.begin() == argv.end(), then you can't get the previous iterator from end() because there is no previous iterator from end(). All of end() - 1, prev(end()), and --end() are undefined behavior already. At that point, we can't even reason about what the loop will do because we don't even have a valid range.

---

What I would suggest instead is:

template <typename It>
struct iterator_pair {
    It b, e;

    It begin() const { return b; }
    It end() const { return e; }
};

// this doesn't work for types like raw arrays, I leave that as
// an exercise to the reader
template <typename Range>
auto drop_last(Range& r) 
    -> iterator_pair<decltype(r.begin())>
{
    return {r.begin(), r.begin() == r.end() ? r.end() : std::prev(r.end())};
}

Which lets you do:

for (const auto& s : drop_last(argv)) { ... }

This is efficient (avoids extra copies), avoids undefined behaviors (drop_last() always gives a valid range), and it's pretty clear what it does from the name.

Answer 2

I find the first option a bit clumsy to read, but as this is rather a matter of personal preference, I propose an alternative approach avoiding a hand-written loop (and under the assumption that argv.size() >= 1), which might be better in the sense that it reduces the likelyhood of typos and indexing bugs.

#include <numeric>

std::string str;

if (!argv.empty())
    str = std::accumulate(argv.begin(), std::prev(argv.end()), str);

Answer 3

My suggestion is to include the guideline support library in your project, if not a more general range-library, and then using a gsl::span (waiting for C++20 to get it as std::span is probably a bit long) or the like for accessing the subrange you want.

Also, it might be small, but it's complicated enough to warrant its own function:

template <class T>
constexpr gsl::span<T> drop_last(gsl::span<T> s, gsl::span<T>::index_type n = 1) noexcept
{ return s.subspan(0, std::min(s.size(), n) - n); }

for (auto s : drop_last(argv)) {
    // do things
}

Actually, taking a look at ranges and views for efficiency (less indirection, no copying) and decoupling (callee no longer needs to know the exact container used) is highly recommended.

Answer 4

Another option is to use std::for_each with a lambda.

std::for_each(argv.begin(), std::prev(argv.end()), [&](const auto& s){ str += s; });

Answer 5

It looks like you are trying to output the elements in a container with a space in between them. Another way to write that is:

const char* space = "";     // no space before the first item
for (const char* s : argv) {
    str += space;
    str += s;
    space = " ";
}

Answer 6

To solve your specific example you could use Google Abseil.

Code example from str_join.h header:

   std::vector<std::string> v = {"foo", "bar", "baz"};
   std::string s = absl::StrJoin(v, "-");
   EXPECT_EQ("foo-bar-baz", s);

EXPECT_EQ is Google Test macro that means that s is equal to "foo-bar-baz"

Answer 7

I'm going to play devil's advocate and say just cast argv.size() to int and use a regular for loop. This is going to work in probably 99% of cases, is easy to read, and doesn't require esoteric knowledge of C++ to understand.

for(int i=0; i<(int)argv.size() - 1; i++)

In the off-chance your container has over 2 billion elements, you will probably know this ahead of time, in which case, just use size_t and use a special case to check for argv.size() == 0 to prevent underflow.

Answer 8

I would recommend using std::prev(v.end()) instead of v.end()-1. That is more idiomatic and works for other containers too.

Here's a main function that demonstrates the idea.

int main()
{
   std::set<int> s = {1, 2, 3, 4};
   for (const auto& item: decltype(s)(s.begin(), std::prev(s.end())))
   {
      std::cout << item << std::endl;
   }

   std::vector<int> v = {10, 20, 30};
   for (const auto& item: decltype(v)(v.begin(), std::prev(v.end())))
   {
      std::cout << item << std::endl;
   }
}

Please note that the above code constructs temporary containers. Constructing a temporary container would be a performance problem when the container is large. For that case, use a simple for loop.

for (auto iter = argv.begin(); iter != std::prev(argv.end()); ++iter )
{
   str += *iter + ' ';
}

or use std::for_each.

std::for_each(argv.begin(), std::prev(argv.end()),
              [](std::string const& item) { str += item + ' '; });