CODEWITHSUNDEEP
c++functionclassvariables
Brunk
Brunk

Reputation: 1

Class function changes other data in class cpp

SOLVED! I Found my error... I posted my solution at the end. Sorry for wasting time.

It has been a while since I have played with c++. It has never been my primary language, so I am not familiar with the finer details. I am having a weird bug pop up. When I call factorize(), it is resetting the sign of the number. This is despite the fact that sign is never touched.

I have found a work around. In my working code, I've added an integer to preserve and reset the value, but I don't think I should have to. I removed those two lines from the code sample below.

Places I reset sign: sign is set to 0 by the constructor of this class. It can be set to 0 in the * and *= operators (if they each have the same sign). It is set to zero by the = operator only if assigning a (unsigned long long) value to the object (it preserves the sign if setting it equal to another FactorNumber).

And that's it. Those are the only places that I am setting sign to zero. I don't see why any of those would be called by this function. But, I am not really attuned to the finer points of how c++ does things when working with classes. I can't see why sign keeps getting reset to 0, but maybe I am doing something wrong. Does anyone see why it is happening?

class FactorNumber {
    private:
        unsigned long long number;
        unsigned long long factor_list[63];     // this is the max possible amount
        int number_of_factors;
        int sign;                               // 0=positive 1=negative
        void factorize();
[snipped irrelevant public function calls]
};    

void FactorNumber::factorize() {
    int x=0;
    for(x=0;x<64;x++) {
        factor_list[x]=0;
    }
    number_of_factors=0;
    unsigned long long current_factor=2;    // 64 bits in c++
    unsigned long long current_number=number;
    unsigned long max_factor=0;  // can never be more than 32 bits
    if (number>3) {
        max_factor=sqrt(current_number);
        while (current_factor<=max_factor) {
            if(current_number%current_factor) {
                if(current_factor>2) {
                    current_factor+=2;
                } else {
                    current_factor=3;   
                }
            } else {
                factor_list[number_of_factors++]=current_factor;
                current_number=current_number/current_factor;
                if(current_number%current_factor) {
                    max_factor=sqrt(current_number);
                }
            }
        }

        // If there is a number larger than one, add it to the array.
        if(current_number>1) {
            factor_list[number_of_factors]=current_number;
        } else {
            number_of_factors--;        // If not, we ignore this last number
        }
    } else {
        number_of_factors=0;
        factor_list[0]=number;
    }
}

My error was an obiwan error. I was writing past the end of my array (factor_list[63] does not actually exist) and that was over-writing my data. It is just coincidence that this reset sign and didn't screw up other stuff (or maybe it does but I didn't catch it yet). This is why I asked the question. It wasn't that I couldn't work around it, I knew there was a bug somewhere in my code.

Changing the for loop condition to x<63 cleared the bug up.

Upvotes: 0

Views: 66

Answers (3)

Brunk
Brunk

Reputation: 1

As has been noted, I was writing past the end of my array.

Upvotes: 0

molbdnilo
molbdnilo

Reputation: 66371

There are 63 items in factor_list.

This loop

for(x=0;x<64;x++) {
     factor_list[x]=0;
}

Writes to 64 long longs.

The last assignment, factor_list[63], overwrites the variables stored after the array, setting sign to 0.

Change the loop index.

You probably also want to add checks that you don't increment number_of_factors too far.

Upvotes: 1

Iosif Murariu
Iosif Murariu

Reputation: 2054

You're overflowing in the 1st for in factorize(), because you're going until index 63, whereas the maximum index (as declared in the class is 62 (size 63)). Actually whenever you call factor_list[X]=Y you have the chance to overflow over the next members in the class. You always need to validate the array index!

unsigned long long factor_list[63]; // <----- indexes from 0 to 62 // <code omitted> factor_list[666] = 0; // <----- Oops! Overflowing (but it's still valid code)

Also, why use C-style arrays instead of C++-style arrays in C++? std::array is a better approach.

Upvotes: 2

Related Questions