Iterating through base class member

Question

I have the something like the following:

#include <vector>
#include <iostream>

template<typename T>
class Vector {
private:
  std::vector<T> base;

public:
  Vector(const std::vector<T> vec) {base = vec;}

  T& operator[](const int& index) {return base[index];}
  std::vector<T> getBase() const {return base;}
};


class BigNum : public Vector<int>
{
public:
  BigNum(const std::vector<int> init) : Vector(init) {}
};


int main()
{
  int arr[] = {6,3,7,6,2};
  std::vector<int> v(arr, arr + sizeof(arr) / sizeof(arr[0]));

  BigNum num(v);

  for(auto it = num.getBase().begin(); it != num.getBase().end(); ++it)
    {
      std::cout << *it << " ";  // What's going on here??
    }

  std::cout << "\n";

  for(int i = 0; i < 5; ++i)
    {
      std::cout << num.getBase()[i] << " ";
    }

  std::cout << "\n";
}

The output of these two loops is:

30134336 0 7 6 2 
6 3 7 6 2

What's going on here? The first number in the first loop (30134336) changes every time, but the remaining numbers are the same. Thanks in advance!

Why are all your arguments marked `const` despite being passed by value? Are you trying to ensure the code is as inefficient as possible? :) — Praetorian, Jan 04 '14 at 07:29

Praetorian · Accepted Answer · 2014-01-04T07:51:09.997

1

 std::vector<T> getBase() const {return base;}

The function returns a copy of the stored vector, so you're iterating across 2 (or a different vector each iteration) completely distinct vectors, which have all been destroyed soon after they were created. Massive undefined behavior. Change the function to

std::vector<T> const& getBase() const {return base;}

I'd rewrite your classes as

template<typename T>
class Vector {
private:
  std::vector<T> base;

public:
  Vector(std::vector<T> vec)
  : base(std::move(vec))
  {}

  T& operator[](int index) {return base[index];}
  T const& operator[](int index) const {return base[index];}
  std::vector<T> const& getBase() const {return base;}
};


class BigNum : public Vector<int>
{
public:
  BigNum(std::vector<int> init) : Vector(std::move(init)) {}
};

And with C++11 you can initialize a vector as

std::vector<int> v{6,3,7,6,2};

edited Jan 04 '14 at 07:51

answered Jan 04 '14 at 07:31

Praetorian

106,671
19
240
328

Thanks! Why did you use `std::move', as opposed to an assignment like there was? – bcf Jan 04 '14 at 07:47
@David What you had was assignment, which would copy the source vector element by element to the destination. [Moving](http://stackoverflow.com/questions/3106110/what-is-move-semantics) is a whole lot more efficient than that. – Praetorian Jan 04 '14 at 07:49
Dunno why I like the term "massive undefined behavior" so much xD. Sorry for spoiled comment. – luk32 Jan 04 '14 at 07:49
@Praetorian Also, what is the the second `const` operator[] needed for? – bcf Jan 04 '14 at 08:08
@David It's not really needed in the example you've posted. But let's say you added a `const` member function to `BigNum` in which you needed to access the inherited `operator[]`, then you'd need that `const` overload. – Praetorian Jan 04 '14 at 08:11
@Praetorian gotcha, and one (hopefully) last question, why is the `const` return type needed for the `getBase()` function? – bcf Jan 04 '14 at 08:30
@David The `const` is not the important change to that function, but returning a reference is. Otherwise a call to `getBase()` returns a *copy* of the vector. Then you call `.begin()` on the temporary vector which gets destroyed immediately, this is UB. Similarly, the `.end()` call on the temporary is UB as well. Basically, you end up iterating over some random memory range. This is why you need to return a reference to the original vector, instead of a copy. The `const` part just ensures clients calling `getBase` are not able to modify the vector through the reference being returned. – Praetorian Jan 04 '14 at 08:40

Oswald · Answer 2 · 2014-01-04T07:45:20.430

0

num.getBase() creates a copy of the base member of Vector.
num.getBase().begin() creates an iterator for that copy.
After the assignement auto it = num.getBase().begin(), the copy that was created by num.getBase() is destroyed. This invalidates the iterator.
Using an invalidated iterator is undefined behaviour. Anything can happen.

You can fix this by defining Vector::getBase() as

const std::vector<T>& getBase() const { return base; }

That way, num.getBase() would return a reference to the original Vector::base, insterad of a copy. You would also have to use std::vector::cbegin(), because begin() would allow you to modify the original vector, which contradicts the const std::vector<T>& return type.

edited Jan 04 '14 at 07:45

answered Jan 04 '14 at 07:31

Oswald

31,254
3
43
68

Thanks, I just changed the `getBase()` function as you have it and it worked, but it still worked without having to use `cbegin()`. Did I miss something? – bcf Jan 04 '14 at 07:50
@David [`vector::begin`](http://en.cppreference.com/w/cpp/container/vector/begin) has an overload that returns a const iterator. So it's not necessary to use `vector::cbegin` (though that would work too). The code would fail after changing `getBase()` if you were to try to modify the vector element using the iterator; for example, `*it = 0;` within the loop will not compile. – Praetorian Jan 04 '14 at 07:56

P0W · Answer 3 · 2014-01-04T07:46:14.070

0

You're invoking a UB

Since auto it = num.getBase().begin() and ,num.getBase().end() are two different vector iterators.

You can use :

  auto v1= num.getBase();
  for(auto it = v1.begin(); it != v1.end(); ++it)
  {
      std::cout << *it << " "; 
  }

Or change getbase() to

std::vector<T> const& getBase() const {return base;}

edited Jan 04 '14 at 07:46

answered Jan 04 '14 at 07:35

P0W

46,614
9
72
119

Iterating through base class member

3 Answers3