Incorrect values for overloading multiplication by scalar for a simple Polynomial class in C++

Question

Simplifying: I am trying to write a simple Polynomial class and I get incorrect values when I try to overload the multiplication operator:

#include <iostream>

class Polynomial {

  public:

    unsigned int degree;
    int *coefficients;

    Polynomial(unsigned int deg, int* arr) {
        degree = deg;
        coefficients = arr;
    }

    ~Polynomial() {}

    int& operator[](int index) const {
        return coefficients[index];  
    }
};

std::ostream& operator<<(std::ostream &os, const Polynomial& P){
    for (unsigned int i=0; i < P.degree; i++) os << P[i] << ",";
    os << P.coefficients[P.degree];
    return os;
}

Polynomial operator*(const Polynomial &P, const int &x) {
    int arr[P.degree];
    for (unsigned int i=0; i <= P.degree; i++) arr[i] = P[i];
    Polynomial p(P.degree, arr);
    std::cout << p << std:: endl; // just for debugging
    return p;
}

I am testing the code in my main:

    int g[] = {-1, 0, 1, 1, 0, 1, 0, 0, -1, 0, -1};
    Polynomial P_g = Polynomial(10, g);

    std::cout << P_g << std::endl;
    Polynomial P_g_3 = P_g*3;

    std::cout << P_g_3[0] << std::endl;
    std::cout << P_g_3[1] << std::endl;
    std::cout << P_g_3[2] << std::endl;
    std::cout << P_g_3[3] << std::endl;
    std::cout << P_g_3[4] << std::endl;
    std::cout << P_g_3[5] << std::endl;
    std::cout << P_g_3[6] << std::endl;
    std::cout << P_g_3[7] << std::endl;
    std::cout << P_g_3[8] << std::endl;
    std::cout << P_g_3[9] << std::endl;
    std::cout << P_g_3[10] << std::endl;
    std::cout << P_g_3 << std::endl;

But the output in the console is something totally different then what I expect:

-1,0,1,1,0,1,0,0,-1,0,-1
-1,0,1,1,0,1,0,0,-1,0,-1
-1
0
0
0
0
0
-2002329776
32767
-516177072
32766
539561192
0,32766,539581697,32767,-2002329776,32767,1,0,243560063,1,-2002329776

Although notice that the inner cout statement from within the overloaded operator returns a correct polynomial. Only when the program exits that function the coefficients get screwed... Moreover the two printing strategies are not even consistent with themselves. What is going on?

Answer 1

Analysis

Your Polynomial class is rather unusual in that it does not own its own data. While this is not inherently wrong, it is almost always an unwise approach.

As an illustration, look at the variables in your main function. The data for the polynomial P_g is stored in the array g. The variable g serves no purpose other than storing that data, so its name is clutter. There is also a consistency concern here, because if someone were to change an element of g, then P_g would also change. Even worse, if g were to cease to exist while P_g was still around, you would have a polynomial without access to its data!

Fortunately, local variables are destroyed in the reverse order of creation, so P_g will be destroyed before g. However, no such luck intervenes when you invoke operator*. In that operator, the polynomial p stores its data in the local variable arr. So far, the situation is the same as in main(). Until you return p. At that point, p is copied/moved into the calling scope, and that copy uses the same data as the original. The array arr gets destroyed, yet the returned object still tries to access arr for its data. The returned object, P_g_3, has a dangling pointer.

When you try to access the data of P_g_3, undefined behavior occurs. In some cases, you might see the behavior you expect. In this case, garbage values were produced. From one perspective, your result is the more desirable one since you were able to detect that a problem exists, which allowed you to attempt to fix it. Far more insidious is the undefined behavior that performs as you expect when you run the program, but not when someone else does.

Solution

The usual approach is to have objects own their own data. Often, this is exclusive ownership, so that the data cannot change without going through the object.

A first refinement would be to move the data-holding arrays into the objects. The main obstacle is that you do not know in advance how large the array is. This calls for dynamic memory allocation. You could start down this road without changing the data members of your class; a first step might be to initialize coefficients to memory allocated with new rather than assigning it arr. This leads to the need to follow the Rule of Three. Unfortunately, this is a bit of work to do correctly, especially since you have chosen to track the degree of the polynomial instead of the size of the array.

Fortunately, storing data of unknown length is a common concern, common enough that the standard library provides tools to automate most of the details. One such tool is std::vector. If you change the type of coefficients from int* to std::vector<int>, then Polynomial objects will be able to own their own data, plus the Rule of Three becomes the Rule of Zero. (That is, the compiler-generated destructor and copy methods will suffice.) Your operator* could simply make a copy of the incoming Polynomial, then iterate over the copy's vector to make changes.

As an added benefit, you no longer need to track degree manually, as the degree of non-zero polynomials will be coefficients.size() - 1. (Well, there is a complication if the leading coefficient is zero, but that's also an unhandled concern for your original implementation.) This demonstrates one reason a class will often keep its data members private. If you had made the data member private and instead defined a degree() method, you could change how the degree is determined without modifying any code that uses Polynomial.

Note:
If you use range-based for loops with your vectors, the example code would have no need to look at the degree of a polynomial. You would be providing the member function for the benefit of code external to the class.