Problem with pointers

Dark_Phoenix · Post by **Dark_Phoenix** » Sat Nov 14, 2009 12:06 pm

I think this may be more of a C++ issue than with wxWidgets but I am out of ideas so I thought I would try here.

I am developing a simple personal finance program. In this program I have a class (CAccountViewer) that simply displays the transactions from a particular account and also allows to enter new transactions. I have another class (CAccountManager) that maintains a list of accounts in a std::map. When the user clicks on an account control transfers to CAccountViewer and the account viewer requests a pointer to the account that the user selected from CAccountManager and saves that pointer as a member variable. This all works fine but when the I attempt to add a new transaction I get a seg fault. Debbuging shows me that the address that the pointer points to becomes invalid somewhere but I can't figure out where. Here is the relivant code.

this is where the user selects an account to view :

Code: Select all

void CHomeFrame::OnListBoxSelect(wxCommandEvent &event)
{
    int sel = m_pAccountListBox->GetSelection();
    if (sel != -1)
    {
        wxString AccountName = m_pAccountListBox->GetString(sel);
        m_StateManager.AddNewState(new CAccountViewer(this, &m_AccountManager, AccountName) );
    }
}

m_StateManager simply switches one pael for another in the sizer

Code: Select all

void CStateManager::AddNewState(wxWindow *newWindow)
{
    // do not push NULL values
    if (newWindow)
    {
        wxWindow *oldWindow = *m_StateList.begin();
        m_StateList.push_front(newWindow);
        m_pParentSizer->Replace(oldWindow, newWindow);
        m_pParentSizer->Layout();
        oldWindow->Show(false);
        newWindow->Show(true);
        newWindow->Refresh();
    }
    else
    {
        SHOWERROR("Cannot push NULL value");
    }
}

control then transfers to the account viewer here :

Code: Select all

CAccountViewer::CAccountViewer(wxWindow *pParent, CAccountManager *pAM, wxString &AccountName)
: wxPanel(pParent, ID_DATAWINDOW)
{
    SetOwnBackgroundColour(wxColour(100, 100, 100) );
    m_Balance = 0.0;
    m_AccountNames = pAM->GetAccountNames();
    m_pAccount = pAM->GetAccount(AccountName);
    m_pGrid = new wxGrid(this, ID_DATAWINDOW, wxDefaultPosition, wxDefaultSize, wxVSCROLL);
    if (!m_pGrid->CreateGrid(0, 9) ) SHOWERROR("Could not create grid");
    m_pGrid->BeginBatch();
      m_pAccount->ResetTransactionCounter();
      while (m_pAccount->HasMoreTransactions() )
      {
          AddNewRow(m_pAccount->GetNextTransaction(), false);
      }
    m_pGrid->EndBatch();
    // set up the button panel
    m_pButtonPanel = new wxPanel(this, ID_BUTTONPANEL);
    m_pButtonPanel->SetBackgroundColour(wxColour(200, 200, 200) );
    // set up the buttons
    wxButton *pBtnCheck    = new wxButton(m_pButtonPanel, IDB_CHECK, wxT("Check") );
    wxButton *pBtnDebit    = new wxButton(m_pButtonPanel, IDB_DEBIT, wxT("Debit") );
    wxButton *pBtnDeposit  = new wxButton(m_pButtonPanel, IDB_DEPOSIT, wxT("Deposit") );
    wxButton *pBtnATM      = new wxButton(m_pButtonPanel, IDB_ATM, wxT("ATM") );
    wxButton *pBtnTransfer = new wxButton(m_pButtonPanel, IDB_TRANSFER, wxT("Transfer") );
    wxBoxSizer *pButtonSizer = new wxBoxSizer(wxVERTICAL);
    pButtonSizer->Add(new wxStaticText(m_pButtonPanel, wxID_ANY, wxT("Enter a new Transaction") ),
                      1,
                      wxALIGN_LEFT | wxALL, 3);
    pButtonSizer->Add(pBtnCheck, 1, wxALIGN_LEFT | wxALL, 3);
    pButtonSizer->Add(pBtnDebit, 1, wxALIGN_LEFT | wxALL, 3);
    pButtonSizer->Add(pBtnDeposit, 1, wxALIGN_LEFT | wxALL, 3);
    pButtonSizer->Add(pBtnATM, 1, wxALIGN_LEFT | wxALL, 3);
    pButtonSizer->Add(pBtnTransfer, 1, wxALIGN_LEFT | wxALL, 3);
    m_pButtonPanel->SetSizer(pButtonSizer);
    wxBoxSizer *pVSizer = new wxBoxSizer(wxVERTICAL);
    pVSizer->Add(m_pGrid, 4, wxALIGN_CENTER | wxALL | wxEXPAND, 5);
    pVSizer->Add(m_pButtonPanel, 1, wxALIGN_CENTER | wxALL | wxEXPAND, 5);
    SetSizer(pVSizer);
    // attach handlers
//....
}

and when one of the new transaction buttons are clicked :

Code: Select all

void CAccountViewer::OnNewTransaction(wxCommandEvent &event)
{
    bool IsXfer = false;
    CTransactionViewer *pTransactionViewer;
    STransaction Transaction;
    Transaction.TransactionNumber = m_pAccount->GetNewTransactionNumber();
    Transaction.TransferNumber = 0;
    Transaction.AccountName = m_pAccount->GetAccountName();
    Transaction.Cleared = TS_OPEN;
//...
}

The seg faults occur when I try to access m_pAcccount in the above code. I don't quite understand this since m_pAccount is a valid pointer in the constructor for CAccountViewer (mem address 0x00b99a28) and then suddenly is not (mem address changes to 0xfdfdfdfd) ??!??

TrV · Post by **TrV** » Sat Nov 14, 2009 12:24 pm

What type is attribute m_pAccount? I guess this is a class which does not define copy constructor and/or operator=().

Dark_Phoenix · Post by **Dark_Phoenix** » Sat Nov 14, 2009 12:40 pm

m_pAccount is a pointer to a CAccount class. I have not defined a copy or assignment constructor. This class only has POD member variables. That is, I am under the impression that wxString and std::vector are considered POD. Is that incorrect?

Code: Select all

class CAccount
{
//...
    private:
    SAccountInfo                         m_AccountInfo;
    wxString                             m_Filename;
    std::vector<STransaction>            m_vTransactions;
    std::vector<STransaction>::iterator  m_CurrentTransaction;
    ULONG                                m_LastTransactionNumber;
    ULONG                                m_LastTransferNumber;
};

struct SAccountInfo
{
    wxString  AccountName;
    wxString  AccountNumber;
    wxString  BankName;
    wxString  BankPhoneNumber;
    wxString  Type;
    wxString  Balance;
    wxString  DateOpened;
};

TrV · Post by **TrV** » Sat Nov 14, 2009 1:13 pm

What is POD ?
Without copy and assignment constructor, it's possible that the affectation "m_pAccount = pAM->GetAccount(AccountName)" leads to some error, even if every CAccount attribute is defined statically (no pointer).
It's possible that default copy for vector and iterator does not copy the whole contents but only the address (i'm not such a STL-guru

).

I think you can also get through this issue by making CAccountManager::GetAccount() return a "const CAccount&", and not a "CAccount".
By the way, what is the protoype for this method?

Dark_Phoenix · Post by **Dark_Phoenix** » Sat Nov 14, 2009 1:52 pm

What is POD ?

Plain Old Data, the way I understand it, it is basically non-dynamically allocated memory.

what is the protoype for this method?

Originally it returned CAccount*, the CAccount object is held in a std::map. I changed it to return a CAccount&.

Code: Select all

void CAccountManager::GetAccount(wxString &name, CAccount &Account)
{
    mapAccountList::iterator iter = m_AccountList.find(name);
    if (iter == m_AccountList.end() )
    {
        SHOWERROR(wxT("Could not find account!") );
        return;
    }
    Account = iter->second;
    return;
}

TrV · Post by **TrV** » Sat Nov 14, 2009 2:06 pm

Dark_Phoenix wrote:
What is POD ?
Plain Old Data, the way I understand it, it is basically non-dynamically allocated memory.

So a vector cannot definitely be POD! Its purpose is to hold any kind of and any number of objects.

Dark_Phoenix · Post by **Dark_Phoenix** » Sat Nov 14, 2009 2:15 pm

Well, I did incorporate a copy and assignment constructor, but the results are the same.

However, I did notice something else. When running in debug, the mem address for CAccountViewer (this) has one value during the constructor. But then the OnNewTransaction meathod is called the address changes and the ULONG member variables change to random numbers. I thought that somehow there was a second copy of CAccountViewer being created so I added a global variable to keep track of how many times the constructor was ran, but this value stayed at '1' for the duration of the program.

How does the address of the this pointer change?

Post by **doublemax** » Sat Nov 14, 2009 2:52 pm

sounds like you used Connect() for the event handlers and didn't use "this" as the event sink parameter.

Unfortunately you didn't post that part of the code.

Dark_Phoenix · Post by **Dark_Phoenix** » Sat Nov 14, 2009 3:06 pm

sounds like you used Connect() for the event handlers and didn't use "this" as the event sink parameter

That's exactly what I did.

So, one more question. Should I use

Code: Select all

wxButton *pBtnCheck = new wxButton(m_pButtonPanel, IDB_CHECK, wxT("Check") );

pBtnCheck->Connect(IDB_CHECK, wxEVT_COMMAND_BUTTON_CLICKED,
                   wxCommandEventHandler(CAccountViewer::OnNewTransaction), NULL, this);

or

Code: Select all

this->Connect(IDB_CHECK, wxEVT_COMMAND_BUTTON_CLICKED,
              wxCommandEventHandler(CAccountViewer::OnNewTransaction) );

Both seem to work fine (right now) but which is correct?

Post by **doublemax** » Sat Nov 14, 2009 3:41 pm

They are both correct.

Dark_Phoenix · Post by **Dark_Phoenix** » Sat Nov 14, 2009 4:56 pm

Thanks for all the help guys!

wxWidgets Discussion Forum

Problem with pointers Topic is solved

Problem with pointers