OOP is killing me

[INeedAHero]

Ok, I read your comments and after closely cross-checking between my .h and .cpp I saw what was wrong and corrected the errors.

Yet, when I try to compile, the same error messages keep coming up! These are the error messages I am seeing:

HugeInteger.cpp: In function âHugeInteger& operator+(const HugeInteger&)â:
HugeInteger.cpp:81:6: error: âarrâ was not declared in this scope
HugeInteger.h:48:14: error: âint HugeInteger::arr [40]â is private
HugeInteger.cpp:81:17: error: within this context
HugeInteger.cpp:81:20: error: âaddâ was not declared in this scope
HugeInteger.cpp:83:11: error: invalid use of âthisâ in non-member function

This is strange to me because I never had a problem with directly accessing arr in the operator+ funciton, but now I'm being warned that it's private. I don't understand that.

You said

You might think about whether you want add to use and return the result in op1 or operate on the instance's arr[] (i.e. take only one argument).

I don't understand how I could have add(...) only take one parameter when its purpose is to add two different arrays, and it isn't supposed to use any objects.

I figure I should post my new .h and .cpp, so you can see the corrections I made:
.h

#ifndef HugeInteger_h
#define HugeInteger_h

#include <string>

//Course class definition
class HugeInteger
{
        public:
                HugeInteger();
                HugeInteger(int[]);

                const HugeInteger & operator + (const HugeInteger&);
                const HugeInteger & operator * (const HugeInteger&);

                friend ostream & operator << (ostream&, const HugeInteger&);
                friend istream & operator >> (istream&, HugeInteger&);

                int atoi(char);
                void display();

         private:
                void add(int[], int[]);
                void mult(int[], int[]);

                static const int MAX = 40;
                static const int MAX2 = 39; // Typing (MAX - 1) is tiresome

                int arr[MAX];
};
#endif 

And .cpp

HugeInteger & operator + (const HugeInteger& right)
{
        HugeInteger result;

        add(arr, right.arr);

        return (*this);
}

void HugeInteger::add(int op1[], int op2[])
{
        int carry;

        for (int i = MAX2; i >= 0; i--)
        {
                op1[i] = op1[i] + op2[i] + carry;

                carry = 0;

                while (op1[i] >= 10)
                {
                        op1[i] -= 10;
                        carry++;
                }

        }
}

[AbstractionAnon]

Consider the following two lines:
From header:
const HugeInteger & operator + (const HugeInteger&);
From the .cpp file:
HugeInteger & operator + (const HugeInteger& right)
Two problems with the implementation:
1) Return type doesn't match (not const).
2) operator is not qualified by the class. Therefore not a member of the class.
Should be:

const HugeInteger & HugeInteger::operator + (const HugeInteger& right)

Because the compiler interpreted operator + as a global function (not a member of your class), the errors are telling you that the function could not access private members (add, arr) of your class.

I don't understand how I could have add(...) only take one parameter when its purpose is to add two different arrays

add is a member of the class so it implictly has access to arr. Passing two arrays to add is okay, I was just pointing out that if the first argument was always arr, it wasn't really necessary to pass it.

[INeedAHero]

Ah, I needed to take a break - spending too many hours in a row programming was causing me to overlook the obvious.

I fixed the errors you pointed out, along with a few other ones that popped up, and I got the program running and adding properly.

But, when I tried multiplying, I was getting a segment fault.

I noticed two errors - one, in my declaration of the array "mult_array[MAX]", I should have mult_array[resultmult.MAX] (line 54 below)
- two, in line 87 (below) I didn't have resultmult.MAX, but rather just MAX.

Yet, even after fixing both those errors (in a way that I believe was correct), I am still getting a segment fault. I have scoured my code for any other problems, but I've come up empty.

Anyone know what can be causing this? It's worth noting that my multiplication algorithm was at one point working without any errors. I have changed none of the logic - I have only converted it into object-based code.

The only potential for a logic error would be in line 90 (in the code below), since I altered that so heavily. Though I doubt the logic itself has been skewed.

Anyone see what's wrong here?

Here's the entire program:

.cpp

#include <iostream>
using namespace std;

#include "HugeInteger.h"

ostream & operator << (ostream & os, const HugeInteger & out)
{
        bool eraseZeroes = true;

        for (int i = 0; i <= out.MAX2; i++)
        {
                if (out.arr[i] != 0 || eraseZeroes == false)
                {
                        os << out.arr[i];
                        eraseZeroes = false;    // The boolean variable eraseZeroes is used to erase superflous zeroes (like 0009 instead of 9)
                }
        }
        return (os);
}

//result.arr[i] = result || arr[i] = op1 || right.arr[i] = op2
const HugeInteger & HugeInteger::operator + (const HugeInteger& right)
{
        HugeInteger result;

        add(right.arr);

        return (*this);
}

void HugeInteger::add(const int op2[])
{
        int carry;

        for (int i = MAX2; i >= 0; i--)
        {
                arr[i] = arr[i] + op2[i] + carry;

                carry = 0;

                while (arr[i] >= 10)
                {
                        arr[i] -= 10;
                        carry++;
                }

        }
}

const HugeInteger & HugeInteger:: operator * (const HugeInteger& rightside)
{
        HugeInteger resultmult;

        int mult_array[resultmult.MAX] = {0};
        int addzero_ctr;
        int lastDigit;
        int firstDigit;
        const int num = 10;
        int product_holder;
        for (int m = (resultmult.MAX2); m >= 0; m--) // This 'for' loop essentially sums together the results of the below 'for' loop to carry out the multiplication
        {
                for (int i = (resultmult.MAX2); i >= 0; i--) // This 'for' loop multiplies all of op1 by the last digit of op2 and puts the value into result[i]
                {
                        product_holder = arr[i] * rightside.arr[m];
                        arr[i] = product_holder;
                        lastDigit = product_holder % num;

                        resultmult.arr[i] = firstDigit;

                        product_holder -= lastDigit;
                        firstDigit = product_holder / num;

                        resultmult.arr[i] += lastDigit;
                }

                if (addzero_ctr != 0)                                   // solution to below
                {
                        for (int i = 0; i <= (resultmult.MAX2 - addzero_ctr); i++)//This is a lot of overhead when addzero_ctr = 0
                        {
                                arr[i] = resultmult.arr[i+addzero_ctr];
                        }
                }


                for (int i = addzero_ctr; i > 0; i--) // Since we need to carry a zero during multiplication, the contents of the array must shift to the left to make room for the zeroes
                {
                        resultmult.arr[resultmult.MAX-i] = 0;
                }

                add(mult_array);

                addzero_ctr++;
                for (int i = 0; i <= resultmult.MAX2; i++)
                {
                        mult_array[i] = arr[i];

                }

        }
} 

And here's .h

 
#ifndef HugeInteger_h
#define HugeInteger_h

#include <string>

//Course class definition
class HugeInteger
{
        public:
                HugeInteger();
                HugeInteger(int[]);

                const HugeInteger & operator + (const HugeInteger&);
                const HugeInteger & operator * (const HugeInteger&);

                friend ostream & operator << (ostream&, const HugeInteger&);
                friend istream & operator >> (istream&, HugeInteger&);

                int atoi(char);
                void display();


         private:
                void add(const int[]);

                static const int MAX = 40;
                static const int MAX2 = 39; // Typing (MAX - 1) is tiresome

                int arr[MAX];
};
#endif   

[INeedAHero]

Anyone see what's going on here?

I'm sorry I can't be more specific with where the problem lies, because I honestly have no idea.

I can only say that it almost certainly has to do with incorrectly using an object. That's the kind of error I would not be able to spot, but one of you who have more experience would be able to see.

[AbstractionAnon]

I do see a few things:

Line 24. result is no longer used.
Line 68. You use firstDigit, but it's uninitialized.
Line 76. You're testing addzero_ctr, but addzero_ctr is uninitialized.
Line 80: addzero_ctr is used as a subscript. Since it's uninitialized you're pretty must guaranteed to get a segmentation fault.
Lines 93-97: I'm not seeing the point of these lines. You're setting mult_array, but mult_array immediately goes out of scope.
.h line 21: Now that you've overloaded <<, you can probably get rid of Display.

[INeedAHero]

Thank you so much!

I initialized everything like you recommended and the program compiles.

Yet I still get a segmentation fault when I run it!

I suspect it may have something to do with lines 93 - 97, the ones you commented on.

The purpose of those lines is to keep memory of the integer formed during multiplication. Because when you multiply longhand, you need to sum two numbers together:
54
X27
----
378
+1080
--------

In that example, I need to remember the 378, so I have mult_array be equal to it, so it can later be added to 1080.

And here's my initialized operator*:

const HugeInteger & HugeInteger:: operator * (const HugeInteger& rightside)
{
        HugeInteger resultmult;

        int mult_array[resultmult.MAX] = {0};
        int addzero_ctr = 0;
        int lastDigit = 0;
        int firstDigit = 0;
        const int num = 10;
        int product_holder = 0;
        for (int m = (resultmult.MAX2); m >= 0; m--) // This 'for' loop essentially sums together the results of the below 'for' loop to carry out the multiplication
        {
                for (int i = (resultmult.MAX2); i >= 0; i--) // This 'for' loop multiplies all of op1 by the last digit of op2 and puts the value into result[i]
                {
                        product_holder = arr[i] * rightside.arr[m];
                        arr[i] = product_holder;
                        lastDigit = product_holder % num;

                        resultmult.arr[i] = firstDigit;

                        product_holder -= lastDigit;
                        firstDigit = product_holder / num;

                        resultmult.arr[i] += lastDigit;
                }

                if (addzero_ctr != 0)                                   // solution to below
                {
                        for (int i = 0; i <= (resultmult.MAX2 - addzero_ctr); i++)//This is a lot of overhead when addzero_ctr = 0
                        {
                                arr[i] = resultmult.arr[i+addzero_ctr];
                        }
                }


                for (int i = addzero_ctr; i > 0; i--) // Since we need to carry a zero during multiplication, the contents of the array must shift to the left to make room for the zeroes
                {
                        resultmult.arr[resultmult.MAX - i] = 0;
                }


                add(mult_array);

                addzero_ctr++;
                for (int i = 0; i <= resultmult.MAX2; i++)
                {
                        mult_array[i] = arr[i];
                }

        }
}

And I figure I may as well show the main.cpp

#include <iostream>
using namespace std;

const int MAX = 40;

#include "HugeInteger.h"
int main()
{
        int op1[MAX] = {0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,4,2,2,0};
        int op2[MAX] = {0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,9,1,0};

        char op;
        op = '*';


//      HugeInteger num1;
//      HugeInteger num2;

//      cin >> num1;
//      cin >> num2;

//      cout << "num1 = " << num1 << endl;
//      cout << "num2 = " << num2 << endl;

        HugeInteger object(op1);
        HugeInteger arr(op2);



//      object.create_array(num1, num2, op1, op2);
        cout << "object: " << object << endl;
        cout << "arr: " << arr << endl;

        if (op == '+')
                arr = arr + object;
        else
                arr = arr * object;

        cout << "The answer is: " << arr << endl;

        return 0;

//      object.display();
}

[INeedAHero]

Segment faults are so tough to deal with!

I can't see the error....

Anyone see anything?

[AbstractionAnon]

Segmentation faults are almost always the result of bad pointers or bad indexes.
Since you're not using pointers, that leaves a bad index.

Regarding my comment about lines 93-97. Please disregard. I failed to notice those lines were within the outermost for loop.

I'll try and look at your code more throughly tonight.

[ne555]

AbstractionAnon wrote:

I also just noticed one thing about both your + and * operators. Both return a HugeInteger by value. When one defines an arithmetic operator, it is normal to both modify the instance and to return the instance by reference.

Your declarations should look like this:

const HugeInteger & operator + (const HugeInteger&);
const HugeInteger & operator * (const HugeInteger&);

¡NO! that's weird behaviour. You should do as the integers.
c = a+b; // ¡¿modifies `a'?!

The prototypes should be

const HugeInteger operator + (const HugeInteger&) const;
const HugeInteger operator * (const HugeInteger&) const;

Maybe you were confused with compound assignment

HugeInteger& operator += (const HugeInteger&);
HugeInteger& operator *= (const HugeInteger&);

[LB]

@ne555: why should the return values of operator+ and operator* be const if they are returned by value?

[ne555]

class foo{
public:
	foo operator+(const foo&) const{}
};

int main(){
	foo a,b,c;
	a+b = c; //compiles
}

[AbstractionAnon]

@ne555 - Why return HugeInteger by value rather than reference? Returning by value is inefficient.

A good anology is string. string::assign and string::append both return the modified string instance by reference.

[LB]

@ne555 is that so wrong? Does it corrupt memory or something?

@AbstractionAnon The problem with returning by reference is that the object you are returning is not kept alive by the reference. Returning by value is generally very efficient in C++11 because of move semantics.

[ne555]

http://www.youtube.com/watch?v=0iWb_qi2-uI form minute 37 to 44
However, I don't think that you could use `move' in this case, as you don't have dynamic allocated arrays.

> both return the modified string instance by reference.
`operator+' should not modify its operands.
Take by instance `std::string' `std::valarray' `int', their operators do not modify their operands.


As an alternative, ban `operator+' and use `operator+='
So in order to say c=a+b you'll use

c=a;
c+=b;

Edit:
@LB: I think it is a logic error, better to not compile then.

[LB]

@ne555 yeah I don't know what I was thinking. Somehow I had it in my head that there were cases where you would want the return value to not be const, but I can't think of any.

[INeedAHero]

I would like to say that I corrected the segmentation fault (I forgot return(*this), haha), and I met with my professor today who told me about some changes I have to make, which I think (hope!) I can do on my own.

I'd also like to thank all the people who came here and helped me with this assignment. You were all a tremendous help and I really appreciate that you took the time to help me. Thank you!