CODEWITHSUNDEEP
c++undefined-behaviorlifetimeobject-lifetimetemporary-objects
passing_through
passing_through

Reputation: 1931

Avoiding undefined behaviour: temporary objects

I've written a class for using it as a convenient view e.g. in range-based fors. Overall, it is just a pair of iterators with bound checking:

template<typename I> class Range {
private:
  I begin;
  I end;

public:
  Range(I const begin, I const end)
    : begin(begin), end(end)
  {}

  Range<I>& skip(int const amount) {
    begin = std::min(begin + amount, end);
    return *this;
  }
};

template<typename C> auto whole(C const& container) {
  using Iterator = decltype(std::begin(container));
  return Range<Iterator>(std::begin(container), std::end(container));
}

Here's the intended usage of it (which led to UB):

std::vector<int> const vector{1, 2, 3};
for (int const value : whole(vector).skip(1)) {
  std::cout << value << ' ';
}

Removing the skip(1) part helps, same about the following refactoring of Range::skip:

Range<I> skip(int const amount) const {
  I const new_begin = std::min(begin + amount, end);
  return Range<I>(new_begin, end);
}

It seems like a temporary mustn't return a reference to itself. Here's what cppreference says:

All temporary objects are destroyed as the last step in evaluating the full-expression that (lexically) contains the point where they were created, and if multiple temporary objects were created, they are destroyed in the order opposite to the order of creation.

Though I'm not sure it's the case and I don't now how to interpret it practically. What's the actual problem and how can I reliably avoid such UB? Are alike expressions e.g. auto string = std::string("abc").append("def") also unsafe?

Upvotes: 2

Views: 359

Answers (3)

songyuanyao
songyuanyao

Reputation: 172894

In ranged-based for loop the range-expression is bound to reference as (prototype code)

auto && __range = whole(vector).skip(1) ;

The problem is the temporary created by whole(vector) is destroyed immediately after the full expression, the reference __range (which binds to the returned reference from skip, i.e. the temporary) becomes dangled; after that any dereference on it leads to UB.

auto string = std::string("abc").append("def") is fine, string gets copied, it's an independent object from the temporary std::string.

Since C++20 you can add init-statement:

If range_expression returns a temporary, its lifetime is extended until the end of the loop, as indicated by binding to the forwarding reference __range, but beware that the lifetime of any temporary within range_expression is not extended.

E.g.

std::vector<int> const vector{1, 2, 3};
for (auto thing = whole(vector); int const value : thing.skip(1)) {
  std::cout << value << ' ';
}

Upvotes: 1

Some programmer dude
Some programmer dude

Reputation: 409166

Regarding std::string and the append function, the append function returns a reference to the string it appends to. Keeping this reference after the string object is destructed will lead to undefined behavior if you use it.

However if you copy the string object, then you're safe, because you will have a copy of the string and not a reference to a non-existing object:

std::string s = std::string("abc").append("def");

Here s will be a copy, initialized through copy-initialization (it passes std::string("abc").append("def") to the copy-constructor for s, and the temporary object will live all throughout that constructor).

As for

for (int const value : whole(vector).skip(1)) { ... }

If the Range<T> class was modified to be iterable (you need a begin and end functions to return the iterators), then it still would not be UB.

That's because such a range-for loop is corresponding to a loop like

for (auto iter = whole(vector).skip(1).begin();
     iter != whole(vector).skip(1).end();
     ++iter)
{
    ...
}

The Range<T> class doesn't contain a copy of the vector, it contains copies of the iterator of the vector (for your shown example). These iterators would be copied or used before the temporary Range<T> object is destructed.

Upvotes: 1

Caleth
Caleth

Reputation: 62636

A range-for holds a reference to the range. Your example

for (int const value : whole(vector).skip(1)) {
  std::cout << value << ' ';
}

is defined to be equivalent to

{
    auto && __range = whole(vector).skip(1);
    auto __begin = __range.begin();
    auto __end = __range.end();
    for ( ; __begin != __end; ++__begin) {
        int const value = *__begin;
        std::cout << value << ' ';
    }
}

The reference returned by skip becomes invalid at the first ;.

Where you are initialising an object, like

std::string s = std::string("abc").append("def");

it is safe, because the temporary only needs to outlive the constructor.

As an aside, I prefer Range<I> skip(int) const over the mutating one.

Upvotes: 1

Related Questions