Writing a C++ Smart Pointer program and having an issue with my = operator

Corps · 09-04-2009, 09:22 PM

I know this isnt the greatest place to post this but this forum has the fastest response times iv found. This is my main file im having an error on the line SmartPtr<int> q = p; Its causing a seg fault and a talking talking about possible corrupted stack so im wondering if im getting some memory leakage. So first is my main code and then my SmartPtr.h file.

#include "SmartPtr.h"
#include <string>
#include <iostream>

using namespace SmartPtrNames;

int main(int argc, char *argv[])
{
// some simple tests
SmartPtr<int> p = new int();
*p = 5;
printAllCounts("first");
cout << "p = " << *p << &p << endl;
SmartPtr<int> q = p;
cout << "p = " << *p << " q = " << *q << endl;
printAllCounts("second");
SmartPtr<int> r = new int();
*r = 2;
printAllCounts("third");
p = r;
cout << "p = " << *p << " q = " << *q << " r = " << *r << endl;
printAllCounts("fourth");
q = p;
cout << "p = " << *p << " q = " << *q << " r = " << *r << endl;
q = p = r = 0;
printAllCounts("fifth");

return 1;
}

namespace SmartPtrNames
{
using namespace std;
void printAllCounts(char* word)
{
};
}

#ifndef _SMARTPTR_H
#define _SMARTPTR_H

template <class T>
class SmartPtr {

public:
SmartPtr(T* ptr = 0) : itsCounter(0)
{
if (ptr) itsCounter = new counter(ptr);
};

~SmartPtr()
{
release();
};

SmartPtr(const SmartPtr<T>& sptr)
{
acquire(sptr.itsCounter);
};

SmartPtr<T>& operator= (const SmartPtr<T>& sptr)
{
if (this != &sptr) {
release();
acquire(sptr.itsCounter);
}
return *this;

};

T* operator->() {};
T& operator*() {};

private:

struct counter {
counter(T* p = 0, unsigned c = 1) : ptr(p), count(c) {}
T* ptr;
unsigned count;
}* itsCounter;

void acquire(counter* c) throw()
{ // increment the count
itsCounter = c;
if (c) ++c->count;
}

void release()
{ // decrement the count, delete if it is 0
if (itsCounter) {
if (--itsCounter->count == 0) {
delete itsCounter->ptr;
delete itsCounter;
}
itsCounter = 0;
}
}

};

#endif /* _SMARTPTR_H */

jstephens84 · 09-04-2009, 09:56 PM

note sure if this would help but wouldn't you need to pass p to q by using something like q = &p. Also you might want to think about creating better variable names. Just a suggestion however.

Just a little rusty on my pointers. May have to break out the old C book again.

johnsfine · 09-04-2009, 11:10 PM

I don't know why these compile at all without any return instruction. But they compile to something very wrong.

Quote:

Originally Posted by Corps

T* operator->() {};
T& operator*() {};

You probably wanted

Code:

T* operator->() { return itsCounter ? itsCounter->ptr : 0; }
T& operator*() { return *itsCounter->ptr; }

Since your operator*() was bad, the line

Code:

*p = 5;

corrupted the stack.
The operator=() was OK but the object being copied had already been corrupted.

Corps · 09-05-2009, 08:54 PM

Thanks for pointing that out I hadn't even realised i hadn't put in functionality for them but now im still getting the same error and it's confusing me more i ran it through the debugger and it highlighted in green above the T* operator-> but when i push step it causes these errors

44 [main] smartpointer 5836 _cygtls::handle_exceptions: Exception: STATUS_A
CCESS_VIOLATION
641 [main] smartpointer 5836 open_stackdumpfile: Dumping stack trace to smar
tpointer.exe.stackdump
18245 [main] smartpointer 5836 _cygtls::handle_exceptions: Exception: STATUS_A
CCESS_VIOLATION
18933 [main] smartpointer 5836 _cygtls::handle_exceptions: Error while dumping
state (probably corrupted stack)

I am programming in windows at the moment if that helps.

dwhitney67 · 09-06-2009, 07:05 AM

Quote:

Originally Posted by Corps

Thanks for pointing that out I hadn't even realised i hadn't put in functionality for them but now im still getting the same error and it's confusing me more i ran it through the debugger and it highlighted in green above the T* operator-> but when i push step it causes these errors

44 [main] smartpointer 5836 _cygtls::handle_exceptions: Exception: STATUS_A
CCESS_VIOLATION
641 [main] smartpointer 5836 open_stackdumpfile: Dumping stack trace to smar
tpointer.exe.stackdump
18245 [main] smartpointer 5836 _cygtls::handle_exceptions: Exception: STATUS_A
CCESS_VIOLATION
18933 [main] smartpointer 5836 _cygtls::handle_exceptions: Error while dumping
state (probably corrupted stack)

I am programming in windows at the moment if that helps.

I'm sorry, but I could not get the code posted from your OP to compile; I had to modify it a bit so that I could compile it, then test it. I believe it is working now.

Main.cpp:

Code:

#include "SmartPtr.h"

#include <iostream>
#include <cassert>

int main(int argc, char* argv[])
{
   using std::cout;
   using std::endl;

   cout << "First test..." << endl;
   SmartPtr<int> p(new int());
   *p = 5;
   assert(*p == 5);

   cout << "Second test..." << endl;
   SmartPtr<int> q = p;
   assert(p == q);
   assert(*p == *q);

   cout << "Third test..." << endl;
   SmartPtr<int> r = new int();
   *r = 2;
   p = r;
   assert(*r == 2);
   assert(r == p);
   assert(*p == 2);

   cout << "Fourth test..." << endl;
   q = p;
   assert(q == p);
   assert(*q == *p);

   cout << "Fifth test..." << endl;
   q = p = r = 0;
   try { assert(q == 0); } catch (std::exception& e) { cout << e.what() << endl; }
   try { assert(p == 0); } catch (std::exception& e) { cout << e.what() << endl; }
   try { assert(r == 0); } catch (std::exception& e) { cout << e.what() << endl; }

   return 0;
}

SmartPtr.h:

Code:

#ifndef _SMARTPTR_H
#define _SMARTPTR_H

#include <stdexcept>

template <class T>
class SmartPtr
{
public:
   SmartPtr(T* ptr = 0) : itsCounter(0)
   {
      if (ptr)
      {
         itsCounter = new counter(ptr);
      }
   }

   ~SmartPtr()
   {
      release();
   }

   SmartPtr(const SmartPtr<T>& sptr)
   {
      if (sptr.itsCounter)
      {
         acquire(sptr.itsCounter);
      }
   }

   SmartPtr<T>& operator=(const SmartPtr<T>& sptr)
   {
      if (this != &sptr)
      {
         release();
         acquire(sptr.itsCounter);
      }
      return *this;
   }

   T* operator->()
   {
      return itsCounter ? itsCounter->ptr : 0;
   }

   T& operator*()
   {
      return *itsCounter->ptr;
   }

   bool operator==(const SmartPtr<T>& other) const
   {
      // there should be a better way to do this!
      if (itsCounter == 0 || other.itsCounter == 0)
      {
         throw std::runtime_error("Illegal attempt to reference a null-pointer!");
      }

      return itsCounter->ptr == other.itsCounter->ptr;
   }

private:
   struct counter
   {
      counter(T* p = 0, unsigned c = 1) : ptr(p), count(c)
      {
      }

      T* ptr;
      unsigned count;
   }* itsCounter;

   void acquire(counter* c) throw ()
   {
      // increment the count
      itsCounter = c;
      if (c) ++c->count;
   }

   void release()
   {
      // decrement the count, delete if it is 0
      if (itsCounter)
      {
         if (--itsCounter->count == 0)
         {
            delete itsCounter->ptr;
            itsCounter->ptr = 0;
            delete itsCounter;
            itsCounter = 0;
         }
      }
   }
};

#endif /* _SMARTPTR_H */

P.S. Thanks johnsfine for your helpful post. I used your code recommendations.

johnsfine · 09-06-2009, 08:00 AM

Quote:

Originally Posted by Corps

Thanks for pointing that out I hadn't even realised i hadn't put in functionality for them but now im still getting the same error

Are you sure you recompiled?

As dwhitney67 said, we can't compile the code you posted. So we know that what you posted is not exactly what you are testing. Without knowing exactly what you are testing, I don't think we can further diagnose your problem.

Quote:

and it's confusing me more i ran it through the debugger and it highlighted in green above the T* operator-> but when i push step it causes these errors

I don't know what debugger you are using, so I don't know what "highlighted in green above" signifies. So I don't know whether you are actually testing operator-> and actually seeing a fault in it, or (more likely) you are misunderstanding that debugger and seeing a fault somewhere else.

Please notice that the program you posted includes no calls to that operator-> and that you instantiate SmartPtr only with a template argument of int. operator-> is meaningless for ordinary (infix) use when the target type is int.
I suggested corrections to both operator-> and operator* but your posted program uses only operator* and when the target type is int, it should use only operator*

Quote:

I am programming in windows at the moment if that helps.

With minimal changes to make it compile (less than dwhitney67 changed) plus the important change to operator* I tested on both Windows (obsolete copy of Visual C++) and Linux (current copy of GCC). Both ran correctly. It should not matter whether you are running on Windows or Linux and for me it didn't.

Notice how dwhitney67 used code tags to make the posted code more readable. Please do that the next time you post any code.

If you still are having trouble, please post code that exactly matches what you are testing.

Corps · 09-07-2009, 07:15 PM

Sorry about the lack of code tags. This is my program now and it compiles and it works. I was using NetBeans and Cygin's g++ compiler. Now i'm working on Linux in Emacs and appreciating the ease of use compared to NetBeans.

Code:

#include "SmartPtr.h"
#include <iostream>

using namespace SmartPtrNames;

// an extremely simple linked list class
class Link {
public:
    SmartPtr<Link> next;
    int data;
};

// a class that takes up a lot of memory
class Big {
public:
    int bigArray[1000000];
};

// a function to test whether scope is handled properly
void testScope()
{
    SmartPtr<Big> b = new Big();
    SmartPtr<Big> b2 = b;
    printAllCounts("testScope");
}

int main(int argc, char *argv[])
{
    // some simple tests
    SmartPtr<int> p = new int();
    *p = 5;
    printAllCounts("first");
    SmartPtr<int> q = p;
    cout << "p = " << *p << " q = " << *q << endl;
    printAllCounts("second");
    SmartPtr<int> r = new int();
    *r = 2;
    printAllCounts("third");
    p = r;
    cout << "p = " << *p << " q = " << *q << " r = " << *r << endl;
    printAllCounts("fourth");
    q = p;
    cout << "p = " << *p << " q = " << *q << " r = " << *r << endl;
    q = p = r = 0;
    printAllCounts("fifth");

    // test a circular linked list, there should be no way of deleting
    // this list - in other words, garbage is created
    SmartPtr<Link> l = new Link();
    l->data = 1;
    assert(l->data == (*l).data);
    l->next = new Link();
    l->next->next = new Link();
    l->next->next->next = l;
    l = 0;
    printAllCounts("after linked list");

    return 1;
}

Code:

namespace SmartPtrNames
{
   #include "SmartPtr.h"

   using namespace std;
   void printAllCounts(char* word)
   {
   };
    
}

#ifndef _SMARTPTR_H
#define	_SMARTPTR_H

template <class T>
class SmartPtr {

public:
    SmartPtr(T* ptr = 0) : itsCounter(0)
    {
        if (ptr) {
            itsCounter = new counter(ptr);
        }
    };

    ~SmartPtr() 
    {
        release();
    };

    SmartPtr(const SmartPtr<T>& sptr)
    {
        if (sptr.itsCounter) {
            acquire(sptr.itsCounter);
        }
    };
    
    SmartPtr<T>& operator= (const SmartPtr<T>& sptr)
    {
        if (this != &sptr) {
            release();
            acquire(sptr.itsCounter);
        }
        return *this;

    };

    T& operator*()  const throw()   {return *itsCounter->ptr;}
    T* operator->() const throw()   {return itsCounter ? itsCounter->ptr : 0;}

    bool unique()   const throw()
        {return (itsCounter ? itsCounter->count == 1 : true);}

private:

    struct counter {
        counter(T* p = 0, unsigned c = 1) : ptr(p), count(c) {}
        T*          ptr;
        unsigned    count;
    }* itsCounter;

    void acquire(counter* c) throw()
    { // increment the count
        itsCounter = c;
        if (c) ++c->count;
    }

    void release()
    { // decrement the count, delete if it is 0
        if (itsCounter) {
            if (--itsCounter->count == 0) {
                delete itsCounter->ptr;
                delete itsCounter;
            }
            itsCounter = 0;
        }
    }


};

#endif	/* _SMARTPTR_H */

The one last issue i have is i want the printAllCounts method to print the location of where the SmartPtr is pointing and i'm frustrated with the namespaces because i want to use cout and endl in the printAllCounts can i pull it out of the namespace and refer to it from in the namespace some how. I took it out and put it at the end of the public methods and added using SmartPtr:

rintAllCounts into the namespace but that caused multiple compile errors

Corps · 09-07-2009, 07:23 PM

Ok newbie mistake forgot to include iostream but any help to print out location would be much appreciated

johnsfine · 09-07-2009, 07:34 PM

Quote:

The one last issue i have is i want the printAllCounts method to print the location of where the SmartPtr is pointing and i'm frustrated with the namespaces because i want to use cout and endl in the printAllCounts can i pull it out of the namespace and refer to it from in the namespace some how. I took it out and put it at the end of the public methods and added using SmartPtr:

rintAllCounts into the namespace but that caused multiple compile errors

I read the above paragraph several times and still have no clue what you're saying nor what you're trying to do.

The way you used printAllCounts in the sample code is not consistent with your phrase "i want the printAllCounts method to print the location of where the SmartPtr is pointing".

I suspect what you want printAllCounts to do would not be possible without maintaining complex additional data structures through the rest of the code. But that is mainly based on a guess of what you want printAllCounts to do. You really haven't made it clear.

Meanwhile, despite the code tags, we still can't guess what your code really looks like, because you posted code that can't compile. You seem to be posting two files, your main cpp file and SmartPtr.h, but that can't match what you have compiled and tested.

Code:

#include "SmartPtr.h"
#include <iostream>

using namespace SmartPtrNames;

The namespace SmartPtrNames should have been declared before the using line. It is not in <iostream> so it must be in "SmartPtr.h".

Code:

namespace SmartPtrNames
{
   #include "SmartPtr.h"

   using namespace std;
   void printAllCounts(char* word)
   {
   };
    
}

#ifndef _SMARTPTR_H
#define	_SMARTPTR_H

You have that chunk of incomplete code posted as if it comes inside SmartPtr.h but before the include guard (which would be very bad practice in any case). It can't actually be inside SmartPtr.h because it does #include "SmartPtr.h". But if it is a third file then the namespace is undefined in the main cpp file. BTW, it is also very bad practice to do a #include inside a namespace.

Corps · 09-07-2009, 08:08 PM

You're right my name space has changed however everything else is the same and does compile and run for me if you try to compile my SmartPtr.h:

Code:

#include <iostream>
#include <cassert>

namespace SmartPtrNames
{

   using namespace std;
   void printAllCounts(char* word)
   {
      int first = 0;
      cout << word << " begin" << endl;
      cout << "location: " << &first << ", count: " << "count goes here" << endl;
      cout << word << " end" << endl;
   };
}

#ifndef _SMARTPTR_H
#define	_SMARTPTR_H

template <class T>
class SmartPtr {

public:
   SmartPtr(T* ptr = 0) : itsCounter(0)
   {
      if (ptr) {
         itsCounter = new counter(ptr);
      }
   };

   ~SmartPtr() 
   {
      release();
   };

   SmartPtr(const SmartPtr<T>& sptr)
   {
      if (sptr.itsCounter) {
         acquire(sptr.itsCounter);
      }
   };
    
   SmartPtr<T>& operator= (const SmartPtr<T>& sptr)
   {
      if (this != &sptr) {
         release();
         acquire(sptr.itsCounter);
      }
      return *this;

   };

   T& operator*()  const throw()   {return *itsCounter->ptr;}
   T* operator->() const throw()   {return itsCounter ? itsCounter->ptr : 0;}

   bool unique()   const throw()
   {return (itsCounter ? itsCounter->count == 1 : true);}

   

private:

   struct counter {
      counter(T* p = 0, unsigned c = 1) : ptr(p), count(c) {}
      T*          ptr;
      unsigned    count;
   }* itsCounter;

   void acquire(counter* c) throw()
   { // increment the count
      itsCounter = c;
      if (c) ++c->count;
   }

   void release()
   { // decrement the count, delete if it is 0
      if (itsCounter) {
         if (--itsCounter->count == 0) {
            delete itsCounter->ptr;
            delete itsCounter;
         }
         itsCounter = 0;
      }
   }


};

#endif	/* _SMARTPTR_H */

And my main is a .cxx if that matters all i've done to it is remove the #include <iostream> from the main but it compiles for me. This is what i want my output to look like:

first begin
location: "address in memory", count: "how many SmartPtrs are pointing to address."
first end

I also think ill use a linked list to hold my SmartPtrs or hold the addresses in memory i also want to add a method that returns the number of pointers pointing to an object and that will be printed out in the printAllCounts or stored in the list i havent yet decided the most efficient way since i'm not sure how to do it.

But this should compile for you.