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);