0

I have a problem with a code (legacy MMO) in VS2008, that I am currently maintaining as a hobby.

Disclaimer: I am not a programmer by trade, aside from C++ my only experience in coding is VBA.

I had problems with dynamically allocated objects (Database Actions using ODBC) in this code since it's being done so frequently, and it's causing heap fragmentation. And I had problems when using boost::shared_ptr with boost::make_shared() and it's causing Heap Corruption.

And I was thinking of removing many of the dynamic allocations and doing something along this lines.

Note: the code had Critical Sections when manipulating the queue, lets say LockOn() enters the Critical Sections and LockOff() leaves the Critical Sections.

class Base
{
public:
    Base(int m_nClientNum):nClientNum(m_nClientNum){}
    virtual ~Base(){}
    virtual void Execute(){std::cout << " Base Executed " << std::endl;
    protected:
    int nClientNum;
};

class Derived: public Base
{
public:
    Derived(int m_nClientNum,int m_nSomeValue, int m_nSomeOtherValue )
    : nClientNum(m_nClientNum),
      nSomeValue(m_nSomeValue),
      nSomeOtherValue(m_nSomeOtherValue)
    {
    }

    virtual ~Derived(){}

    virtual void Execute()
    {
        std::cout << "Client Num: " << nClientNum << " , Value : " << nSomeValue << " ,Other Value: " << nSomeOtherValue << std::endl;
    }
    
protected:
    int nClientNum;
    int nSomeValue;
    int nSomeOtherValue;
};

class Consumer
{
public:
    Consumer(){}
    ConsumeBaseNow(Base* m_Base)
    {
        m_Base->Execute();
    }
    
    // The code originally only has the following two Functions
    void AddBaseQueue(Base* m_Base)
    {
        LockOn();
        m_vBase.push(m_Base);
        LockOff();
    }
    
    void ConsumeBaseLater()
    {
        Base* pTemp = NULL; // NULL being defined as 0 in the standard header.
        LockOn();
        if ( m_vBase.IsEmpty() )
        {
            Sleep(1);
            LockOff();
        }
        else
        {
            pTemp = m_vBase.front();
            m_vBase.pop();
            LockOff();
            if ( pTemp )
            {
                pTemp->Execute();
                delete pTemp;
                pTemp = NULL;
            }
        }
    }
    
protected:
    std::queue<Base*> m_vBase;
};

// For simplicity's sake, I'm using the main function to show how the code is handling this class.
int main()
{
    // Here is how the logic goes for the original Code
    Consumer* pConsumer = new Consumer;
    Derived* pDerived = new Derived(10,20,30);
    pConsumer->AddBaseQueue( pDerived );
    pConsumer->ConsumeBaseLater();

    // Now my plan is to do this, since some of these Actions don't need 
    // to be queued since they are not being used that frequently and together at the same time.
    Derived pDerived2(40,50,60);
    Base* pBase = &pDerived2;
    pConsumer->ConsumeBaseNow(pBase);

    delete pConsumer;
    pConsumer = NULL;
}

My questions are as follows:

  1. Is creating a pointer that is pointing to an object on the stack undefined behavior? Is it a common thing to do, or is it something that I should put in a box and throw away in the lake?

  2. Is there a problem with my boost library (1.43) that is causing heap corruption (memory keeps growing fast until I run out of it)? Note: I'm using it this way:

    boost::shared_ptr<Base> pBase = boost::make_shared<Derived>(10,20,30); 
    

    Then I changed ConsumeBaseLater with this:

    {
        LockOn();
        if ( m_vBase.IsEmpty() )
        {
            Sleep(1);
            LockOff();
        }
        else
        {
            boost::shared_ptr<Base> pTemp ( m_vBase.front() );
            pTemp->Execute();
            m_vJob.pop();
            LockOff();
        }
    }
    

    Or, am I using shared_ptr the wrong way?

  3. If I changed the queue to accept the objects themselves, how do I do it without slicing the objects? Since the queue is accepting the base class, and as far as my understanding goes, all derived classes are bases, but not all bases are derived.

EDIT: Thanks to Paul for pointing out that I didn't have virtual destructors in my original question's code (the code I'm maintaining does have virtual destructors, it's the first thing I looked at when I first encountered the heap problems), and I simply forgot to write it in the original question.

Remy Lebeau
  • 555,201
  • 31
  • 458
  • 770
  • Your `Base` class lacks a virtual destructor, thus is unsafe to be used polymorphically. – PaulMcKenzie Apr 13 '22 at 20:48
  • `delete pTemp;` -- Undefined behavior. No need to look further until you fix the virtual destructor issue. – PaulMcKenzie Apr 13 '22 at 20:51
  • 2
    *Is creating a Pointer that is pointing to an Object on the Stack an undefined behavior?* Nope. Perfectly fine. Just don't try to `delete` it. We generally prefer the term automatic to stack because automatic better describes the management of the instance. It's all handled for you automatically. – user4581301 Apr 13 '22 at 20:52
  • Paul's effectively covered point 2. – user4581301 Apr 13 '22 at 20:53
  • Also, I bet the issue is not with `boost`. If it was, thousands of persons would have reported issues with it. – PaulMcKenzie Apr 13 '22 at 20:53
  • 1
    Point3 is a concern. You cannot avoid slicing with a queue of instances rather than a queue of references. Perhaps the concept of [Composition Over Inheritance](https://en.wikipedia.org/wiki/Composition_over_inheritance) may apply here. – user4581301 Apr 13 '22 at 20:55
  • @Paul I'm very sorry for the mistake in the code above, the Original code had virtual destructors, I just didn't wrote it rather forgot to., I've edited the question reflect it. it's 5:00 AM as of the time i'm writing this in my country :). – Azriel Elijay Apr 13 '22 at 20:55
  • 2
    @AzrielElijay Also, using `LockOn` and `LockOff` as fence-posts in multithreaded C++ applications is not recommended. What if during the `LockOn`, an exception is thrown? You never get to the `LockOff`, thus a deadlock. What is done is to wrap the `LockOn/LockOff` in a small object that when destroyed, calls `LockOff` automatically. This is what [RAII](https://stackoverflow.com/questions/2321511/what-is-meant-by-resource-acquisition-is-initialization-raii) is all about. – PaulMcKenzie Apr 13 '22 at 21:01
  • @PaulMcKenzie Thank you very much for that information , When i looked at the code itself the LockOn() method is a Base Class of the Consumer Class and I thought it was implementing RAI already. would I be right to assume that the proper way would be to remove the inhertiance from the CLock Object and and instead Created the CLock Object on the function needing the Critical section instead. – Azriel Elijay Apr 13 '22 at 21:08
  • @user4581301 Thank you very much as I am only 6 Months into exploring C++ and it's capabilities with very limited theory behind me ( Never found a Good book about C++ that is both Good and Entertaining like what I had for VBA ) or maybe I shouldn't look for once since with my time coding this 1500 file Source, It's beginning to dawn on me that C++ can't be taken half arsed like other languages. But it's Speed is really amazing! Even for the C++ Version being used in VS2008. – Azriel Elijay Apr 13 '22 at 23:02
  • 1
    Someone would eventually prove me wrong if I said C++ is too complicated to write an entertaining book that covered the language well, but it hasn't happened yet, to my knowledge, and will take a LOT of work. I mean, smurf, [look what they have to make fun](http://eel.is/c++draft/). – user4581301 Apr 13 '22 at 23:12
  • 1
    When you've slogged through the language fundamentals, give [Effective Modern C++](https://www.amazon.com/dp/1491903996) a read. Most of it is still relevant today. – user4581301 Apr 13 '22 at 23:14

0 Answers0