1

Consider the following implemntation of a Binary class representation of an integer:

class Binary {
private:
    int *digits;
    int first1;
public:
    Binary() {
        digits = new int[128];
        digits[0]=0;
        first1=0;
    }
    ~Binary() {
        cout<<"deleting"<<endl;
        delete [] digits;
    }
    Binary(const Binary& b){
        digits = new int[128];
        memcpy(digits,b.digits,128*sizeof(int));
        first1=b.first1;
    }
    explicit Binary(double x){
        int n=(int)x,i;
        digits = new int[128];
        for (i=0; n ; i++,n>>=1) digits[i]=(n & 1)? 1:0;
        first1=i-1;
    }
    Binary& operator =(const Binary& b){
        if (this==&b) return *this;
        memcpy(digits,b.digits,128*sizeof(int));
        first1=b.first1;
        return *this;
    }
    Binary(int n) {
        int i;
        digits = new int[128];
        for (i=0; n ; i++,n>>=1) digits[i]=(n & 1)? 1:0;
        first1=i-1;
    }
    void print() {
        for (int i=first1; i>=0 ; i--) cout<<digits[i];
        cout<<endl;
    }
    operator int() {
        int x = 1,sum=0;
        for (int i=0; i<=first1 ; i++,x<<=1) sum+=digits[i]*x;
        return sum;
    }
    Binary& operator +(Binary& a) {
        int overflow = 0;
        Binary* b1=new Binary();
        int max = a.first1>this->first1? a.first1 : this->first1,bit;
        for (int i=0; i<=max ; i++) {
            bit=a.digits[i]+this->digits[i]+overflow;
            overflow=0;
            if (bit>1) overflow=1;
            b1->digits[i]=bit%2;
        }
        return *b1;
    }

};

and the main using it:

int main() {
    Binary b(91);
    int x=9;
    Binary *b2=new Binary();
    *b2=b+x;
    x=*b2;
    b2->print();
    cout<<" = "<<x;
    cin>>x;
}

lets talk about the line:

*b2=b+x;

first the compiler implicitly allocates a new binary instance for int x, then using it as a paramater for the addition, then creates a new binary instance for the addition result and copies it bit by bit to *b2.

The problem is, that if you run this code, it only prints deleting ONCE, while there were 2 objects created for the execution of the command. apparently there's a leak comes from the addition code, in which i explicitly created a new object to return the result.

Q1: am i correct?

Q2: what can i do to overcome this?

EDIT: The answer and more about the topic of operator overloading can be found here

Community
  • 1
  • 1
Ofek Ron
  • 8,354
  • 13
  • 55
  • 103
  • 3
    Your `operator+` should return `Binary`, not `Binary&` and it should not dynamically allocate an object. – James McNellis Jul 05 '12 at 21:25
  • I'm curious why it even compiled because you don't have overloaded operator+(int) – Blood Jul 05 '12 at 21:25
  • 1
    @Blood: He has an implicit conversion constructor from int, so an object is created, then passed to `operator+`. – Benjamin Lindley Jul 05 '12 at 21:27
  • 1
    In regards to **Q2**, start by replacing `int* digits` with `std::vector digits` and eliminating all calls to `new` and `delete`. – Benjamin Lindley Jul 05 '12 at 21:33
  • Also, it looks like you lose the carry out of the top bit in your + routine. – stark Jul 05 '12 at 21:42
  • 1
    Constructor from `double` is wrong: `int` has usually 32 bits (even on most x64 platforms – it depends on compiler) and `double` is 64-bit. And also (that's what I've thought after seeing your class name and it got me confused) – this would be wrong behaviour: your class says, that it contains given number in binary, so just casting it to `int` isn't expected at all. You should show binary representation of floating-point number, not just integer parts of it in U2. – Archie Jul 05 '12 at 23:12
  • @Archie, i didnt mention it because its off topic, but the task was to present the floor of a double, therefore int was simple enough. – Ofek Ron Jul 06 '12 at 00:20
  • @James, I agree that it would work, but then you wont be able to do something like A=B=C, which ref allows. – Ofek Ron Jul 06 '12 at 00:21
  • Would be nice to see some comments in code – newprint Jul 06 '12 at 00:24
  • 1
    @OfekRon: James said that `operator+` should return by value not reference, and he's right. How does that relate to `A=B=C`? – Mooing Duck Jul 06 '12 at 00:29

3 Answers3

2

Summary: Objects allocated with new must be deleted with delete. Objects allocated with new[] must be deleted with delete[]. Globals and locals are deleted automatically when their scope/TU execution ends. In Binary& operator +(Binary& a) you make a Binary that is leaked, and in main you make another Binary that is leaked.

These problems would be avoided if wrote operator+ like so:

Binary operator +(Binary& a) const{ //return by value
    Binary b1(*this); //hold it on the stack
    //math here
    return b1; //return by value
}

and if in main you avoided allocation as well:

Binary b2 = b+x;
x = b2;
b2.print();

This will be faster than your original code, is easier to read and understand, and won't leak.

[Other notes]

Use a std::vector for the internal data instead of managing your own dynamic array. vector is easier, and less likely to make mistakes.

It's usually best to make conversions (like int -> Binary) explicit wherever you can. It adds typing, but saves headaches. This goes for your int conversion operator as well.

Make your const functions const. Right now, if you get a const Binary, you can't do almost anything with it. You can't print it, you can't add anything to it...

You appear to be storing one bit per int, which means you're using about 97% more space than needed (wasting 99.99999995% of the values), which is just silly. Most novices start with 0-9 per char, which only wastes 50% of the space. (though that's still 96% of the values), but is really easy to understand.

The normal way to do addition is like this:

Binary& operator+=(const Binary& rhs) { 
    int max = a.first1>this->first1? a.first1 : this->first1,bit;
    for (int i=0; i<=max ; i++) {
        bit=a.digits[i]+this->digits[i]+overflow;
        overflow=0;
        if (bit>1) overflow=1;
        b1->digits[i]=bit%2;
    }
}  
Binary friend operator+(Binary lhs, const Binary& rhs) {  
{return lhs+=rhs;}
Mooing Duck
  • 64,318
  • 19
  • 100
  • 158
  • Great, thanks!, one more thing tho, I added delete to the allocation in main and used a some plugin to check if there were any leaks and it said there arent any, how come? – Ofek Ron Jul 06 '12 at 01:02
  • and why did u declare it freind? – Ofek Ron Jul 06 '12 at 01:03
  • @OfekRon: if you have two `new` and only one `delete`, then the plugin is wrong. I declared it friend so that the left side can take advantage of conversions. That's the normal way to do it. With `operator+=` as a member taking one argument, and `operator+` as a `friend` taking two arguments. – Mooing Duck Jul 06 '12 at 01:56
0

If you really see "deleting" only once, than this must be the variable b. *b2 = b+x may be done by converting b to int, adding x, constructing a Binary from it and copying that to the place where b2 points to. As b2 is just a raw pointer, you are leaking the initial *b2 instance and the one that overwrites it.

Arne
  • 2,106
  • 12
  • 9
0

You're not deleting b2 at the end of main.

Also operator should return by value, not returning an allocated Binary object as that will leak because C++ is not garbage collected

Also operator+ should take a const reference.

Also, print, operator+ and operator int should be const member functions

Also, you don't need to dynamically allocate the 128 ints, just make it an array of 128 ints

private:
    int digits[128];

and remove the delete on digits

also, you should initialize it in the constructor with

memset(digits, 0, sizeof(digits));
cppguy
  • 3,611
  • 2
  • 21
  • 36