Speicherprobleme beim verwenden einer verketteten Liste

Hallo,
bei meinem Versuch tiefer in die Welten der Objektorientierung vorzudringen, bin ich auf das Problem der Datenspeicherung gestossen, und dachte eine verkette Liste von Objekten währe für mich die ideale Lösung.
Leider bekomme ich an einer Stelle immer während der Ausführung (gdb) einen "Segmentation Fault", und hab leider keine Ahnung warum. Vielleicht könnte jemand aus diesem Forum mir einen Hinweis geben, wo denn der grundlegende Fehler in meiner Implementation liegt ?

Zunächst das Objekt, das an/in die Liste an/eingehängt werden soll:

class CWare{

        protected:

            int     iWareIndex;                                 //Interner Index für die einzelnen Warentypen
            char    cWareName[WARE_NAME_LENGTH];                //Bezeichnung der Ware z.B "Mammutpelz"
            int     iWarePriceSell;                             //Verkaufspreiss der Ware
            int     iWarePriceBuy;                              //Kaufpreiss der Ware
            int     iWareAmount;                                //Warenmenge
            int     iWareWeight;                                //Gewichtseinheiten, die die Ware einnimmt (hat nur in Waggons Bedeutung)
            int     iWareVolume;                                //Volumenseinheiten, die die Ware einnimmt (hat nur in Waggons Bedeutung)
            int     iWareStorageType;                           //Welche Art der Lagerung erfordert die Ware z.B. Flüssigkeiten => Tank
            int     iWareStaleFactor;                           //Ist die Ware verderblich, und wenn ja wieviele Zeiteinheiten kann sie gelagert werden

            //CWare   *pFirstWare;
            //CWare   *pNextWare;                                 //Zeiger auf nächste Ware in der jeweiligen Liste
            //CWare   *pPreviousWare;                             //Zeiger auf vorherige Ware in der jeweiligen Liste
            //CWare   *pLastWare;

        public:

            CWare   *pNextWare;                                 //Zeiger auf nächste Ware in der jeweiligen Liste
            CWare   *pPreviousWare;                             //Zeiger auf vorherige Ware in der jeweiligen Liste

            CWare () {pNextWare = NULL; pPreviousWare = NULL;}
            //CWare () {pFirstWare = NULL; pNextWare = NULL; pPreviousWare = NULL; pLastWare = NULL;}
            CWare (int Index,char *Name, int PriceSell, int PriceBuy,int WareAmount, int WareWeight, int WareVolume, int WareStorageType, int WareStaleFactor);

            CWare*  WareAdd(int Index, char *Name);

            void    WareSetIndex(int iNewWareIndex) {iWareIndex = iNewWareIndex;}
            void    WareSetName(char *cNewWareName) {strncpy(cWareName,cNewWareName,WARE_NAME_LENGTH);}
            void    WareSetPriceSell(int iNewWarePriceSell) {iWarePriceSell = iNewWarePriceSell;}
            void    WareSetPriceBuy(int iNewWarePriceBuy) {iWarePriceBuy = iNewWarePriceBuy;}
            void    WareSetAmount(int iNewWareAmount) {iWareAmount = iNewWareAmount;}
            void    WareSetWeight(int iNewWareWeight) {iWareWeight = iNewWareWeight;}
            void    WareSetVolume(int iNewWareVolume) {iWareVolume = iNewWareVolume;}
            void    WareSetStorageType(int iNewWareStorageType) {iWareStorageType = iNewWareStorageType;}
            void    WareSetStaleFactor(int iNewWareStaleFactor) {iWareStaleFactor = iNewWareStaleFactor;}

            int     WareGetIndex(void) {return iWareIndex;}
            void    WareGetName(char *cName) {strncpy(cName,cWareName,WARE_NAME_LENGTH);}
            int     WareGetPriceSell(void) {return iWarePriceSell;}
            int     WareGetPriceBuy(void) {return iWarePriceBuy;}
            int     WareGetAmount(void) {return iWareAmount;}
            int     WareGetWeight(void) {return iWareWeight;}
            int     WareGetVolume(void) {return iWareVolume;}
            int     WareGetStorageType(void) {return iWareStorageType;}
            int     WareGetStaleFactor(void) {return iWareStaleFactor;}

            //void    WareSetNext(CWare *pNewNextWare) {pNextWare = pNewNextWare;}
            //CWare*  WareGetNext(void){ return pNextWare;}

            //int     WareGetSpecificationFromFile(int iWareIndex);

            //int     CreateGoods(char *CityName);

    };

Die Liste die die einzelnen Objekte Aufnimmt sieht folgendermaßen aus
Header:

class CWareList{

        protected:

            CWare   Ware;

            CWare   *pFirstWareInList;
            CWare   *pLastWareInList;

        public:

            CWareList () {pFirstWareInList = NULL; pLastWareInList = NULL;}
            void    WareListItemAdd(int Index, char *Name);
            void    WareListItemGet(int Index, char *Name, int *PriceSell, int *PriceBuy,int *WareAmount, int *WareWeight, int *WareVolume, int *WareStorageType, int *WareStaleFactor);
            void    WareListItemRemove(int Index, char *Name);
            void    WareListClear(void);
    };

Implementation:

void CWareList::WareListItemAdd(int Index,char *Name){

    if(pFirstWareInList == NULL)
    {
        pFirstWareInList = new CWare(Index,Name,-1,-1,1,1,1,1,1);
        pLastWareInList = pFirstWareInList;
        pLastWareInList->pNextWare = NULL;
        pLastWareInList->pPreviousWare = NULL;
    }
    else
    {
        pLastWareInList->pNextWare = new CWare(Index,Name,-1,-1,1,1,1,1,1);
        pLastWareInList->pNextWare->pPreviousWare = pLastWareInList;
        pLastWareInList = pLastWareInList->pNextWare;
        pLastWareInList->pNextWare = NULL;
    }

}

void CWareList::WareListItemGet(int Index, char *Name, int *PriceSell, int *PriceBuy,int *WareAmount, int *WareWeight, int *WareVolume, int *WareStorageType, int *WareStaleFactor){

    int     iSearchByChar=0,
            iFound=0,
            i;
    char    cTempName[WARE_NAME_LENGTH];
    CWare*  thisObject = pFirstWareInList;

    if (Index == -1) iSearchByChar = 1;
    else iSearchByChar = 0;

    while(thisObject!=NULL){
        if (iSearchByChar == 0){ //suche nach Index
            i = thisObject->WareGetIndex();
            if(i == Index){
                iFound = 1;
                break;
            }
        }
        else{// suche nach Warenname
            thisObject->WareGetName(cTempName);
            if (strcmp(cTempName,Name) == 0){
                iFound = 1;
                break;
            }
        }
        thisObject = thisObject->pNextWare; //zum nächsten Objekt springen
    }
    if (iFound == 1){
        //*Index = thisObject->WareGetIndex();
        thisObject->WareGetName(cTempName);
        strncpy(Name,cTempName,WARE_NAME_LENGTH);
        *PriceSell = thisObject->WareGetPriceSell();
        *PriceBuy = thisObject->WareGetPriceBuy(); // !!!!! <--SegFault!!!!!!!!!
        *WareAmount = thisObject->WareGetAmount();
        *WareWeight = thisObject->WareGetWeight();
        *WareVolume = thisObject->WareGetVolume();
        *WareStorageType = thisObject->WareGetStorageType();
        *WareStaleFactor = thisObject->WareGetStaleFactor();
    }
}

void CWareList::WareListClear(void){

    CWare* thisObject = pFirstWareInList;
    CWare* deadObject = NULL;

    while(thisObject!=NULL)
    {
        deadObject=thisObject;
        thisObject=thisObject->pNextWare;
        delete deadObject;
    }

    pFirstWareInList = NULL;
    pLastWareInList = NULL;
}

Aufgerufen wird das Ganze dann folgendermaßen:

CWareList wMoskauWareList;

    wMoskauWareList.WareListItemAdd(1,"Brot");

    wMoskauWareList.WareListItemAdd(2,"Senf");

    wMoskauWareList.WareListItemGet(1,cName,iPriceSell,iPriceBuy,iAmount,iWeight,iVolume,iStorageType,iStaleFactor);
    wMoskauWareList.WareListClear();

Es spielt hierbei auch keine Rolle, wieviele Objekte ich an die Liste anhänge (oder es zumindest versuche), oder welches Element ich mir dann gerne näher betrachten möchte

Für jeden kleinen Hinweis währe ich sehr dankbar

Phoemuex

Ich habe gerade nicht genug Zeit, mir den ganzen Code durchzulesen

Aber wenn du die Liste nicht unbedingt selber Schreiben willst, würdfe ich dir raten, dir mal die Klasse std::list aus der STL anzusehen.

Kleines Beispiel:

#include<list>

using namespace std;

int main(void)
{
    list<CWare> eineListe;
    eineListe.pusBack(CWare(...)); //Keine Ahnung welche Argumente der Konstruktor nimmt
    //... noch mehr Sachen einfügen
    for(list<CWare>::iterator it = eineListe.begin(); it != eineListe.end(); ++it)
    {
        //it kann hier verwendet werden, wie ein Zeiger auf CWare
    }
    return 0;
}

DJohn

Hast Du für die Pointer cName, iPriceSell,iPriceBuy u.s.w. auch Speicher reserviert?

DJohn

@ Phoemuex:
Hätte wohl dazuschreiben sollen, daß ich keine STL verwenden möchte - zum einen wegen der Portabilität (wobei das ein fragwürdiges Paradigma zu sein scheint, das ich aus der WxWidgets - Doku aufgesogen habe) zum anderen, soll der Lerneffeckt ganz oben stehen (versuche gerade mich in OO - Programmierung einzuarbeiten)
Trotzdem Danke

@ DJohn
Bin etwas verwirrt, meinst du die Pointer, die im Funktionskopf von "WareList::WareListItemGet" definiert werden ? Ich dachte die leben nur zusammen mit der Funktion, und ich wüßte auch nicht so recht wie und warum ich Speicher reservieren soll - ich habe immer Gedacht ich definiere einen Int -> Compiler reserviert Platz für einen Int im Speicher, ich definiere einen Zeiger auf einen Int -> Compiler reserviert Platz für einen Zeiger auf einen Int im Speicher.

Mist, naja, ist vielelicht auch etwas viel verlangt, daß jemand meinen Murkscode auseinanderklamüsert. Hmm hat vielleicht jemand einen fertigen Quelltextschnipsel, um ein Objekt in einer verketteten Liste zu speichern (zum abkucken, und auseinandernehmen) oder vielleicht eine bessere/intelligentere/effizientere Idee, wie ich an ein Objekt dynamisch eine Liste anderer Objekte anhänge ?

Thomas ** 0

Gastfrager schrieb:

@ Phoemuex:
Hätte wohl dazuschreiben sollen, daß ich keine STL verwenden möchte - zum einen wegen der Portabilität (wobei das ein fragwürdiges Paradigma zu sein scheint, das ich aus der WxWidgets - Doku aufgesogen habe) zum anderen, soll der Lerneffeckt ganz oben stehen (versuche gerade mich in OO - Programmierung einzuarbeiten)
Trotzdem Danke

Dann bau dir eine eigene Template-List-Klasse, so ist das absoluter CRAP!

hustbaer

@Gastfrager:

"Intrusive lists" vertragen sich nicht gut mit OOP. "Intrusive" heisst dabei dass "die Liste" quasi in das "zu Listende" eindringt, die Klasse deren Objekte du listen willst (das "zu Listende") muss hier ja "next" und "prev" Pointer haben (was quasi ein Teil "der Liste" ist).

Ansonsten... der Code ist grauenhaft (Benamsung etc.). Fang einfach ganz klein an, mal bloss die Liste und sonst nix, und füg Schritt für Schritt Funktionalität dazu, dann bekommst du am einfachsten raus wo es knallt. Oder nimm ne Entwicklungsumgebung in der man gut debuggen kann, MSVC unter Windows z.B.

Und wenn du schon mit "intrusive lists" arbeiten willst, dann würde ich dir empfehlen eine Basisklasse "ListNode" zu machen, und eine Klasse "List", die eben "ListNode" Instanzen "listen" kann. Dann leitest du deine Elemente einfach von "ListNode" ab, und gibst der Klasse die diese Elemente listen soll ein Member "List".

DJohn

Gastfrager schrieb:

@ DJohn
Bin etwas verwirrt, meinst du die Pointer, die im Funktionskopf von "WareList::WareListItemGet" definiert werden ?

Nein, ich meine die Pointer, die Du verwendest unter:

Gastfrager schrieb:

Aufgerufen wird das Ganze dann folgendermaßen:

Dort zeigst Du leider nicht den Teil, wo Du die Variablen definierst.

Gastfrager schrieb:

und ich wüßte auch nicht so recht wie und warum ich Speicher reservieren soll - ich habe immer Gedacht ich definiere einen Int -> Compiler reserviert Platz für einen Int im Speicher, ich definiere einen Zeiger auf einen Int -> Compiler reserviert Platz für einen Zeiger auf einen Int im Speicher.

Ja, das ist richtig.
Aber bei einem Zeiger wird eben nur der Platz für den Zeiger selbst automatisch reserviert, nicht aber der Platz auf den der Zeiger zeigt (den Int)!
Und in Deiner WareListItemGet()-Methode (Dort wo der Seg-Fault auftritt), veränderst Du ja nicht den Zeiger selbst, sondern Du dereferenzierst den Zeiger mit *.

Du hast 2 Möglichkeiten, entweder Du definierst Variablen vom Typ int und übergibst die Adresse auf diese Variablen an WareListItemGet():

char cName[WARE_NAME_LENGTH];
int iPriceSell;
int iPriceBuy;
int iAmount;
int iWeight;
int iVolume;
int StorageType;
int iStaleFactor;

wMoskauWareList.WareListItemGet(1,&cName[0],&iPriceSell,&iPriceBuy,&iAmount,&iWeight,&iVolume,&iStorageType,&iStaleFactor);

oder Du definierst Pointer und reservierst den Speicher selber.

char* cName=new char[WARE_NAME_LENGTH];
int* iPriceSell=new int;
int* iPriceBuy=new int;
int* iAmount=new int;
int* iWeight=new int;
int* iVolume=new int;
int* StorageType=new int;
int* iStaleFactor=new int;

wMoskauWareList.WareListItemGet(1,cName,iPriceSell,iPriceBuy,iAmount,iWeight,iVolume,iStorageType,iStaleFactor);

// mit den Werten arbeiten:
...
// und Speicher wieder freigeben
delete[] cName;
delete iPriceSell;
delete iPriceBuy;
delete iAmount;
delete iWeight;
delete iVolume;
delete iStorageType;
delete iStaleFactor;

Für cName ist das Ganze noch etwas komplizierter, da du hier zwar einen Zeiger auf einen char übergibst, aber eigentlich ein Array von chars erwartest. Besser und einfacher, wäre die Verwendung von std::string.

Ich habe mich Durch Deinen Code gekämpft, und soweit ich es verstanden habe, scheint die Liste selbst zu funktionieren.

DJohn

volkard

hustbaer schrieb:

@Gastfrager:
"Intrusive lists" vertragen sich nicht gut mit OOP.

war es nicht früher mal so, daß intrusive lists (besonders die doppelt verketteten ringe) mit dicken großen members die stl-listen wegen weniger new/delete plattmachen?
erklär mal und widerlege dich.

Und wenn du schon mit "intrusive lists" arbeiten willst, dann würde ich dir empfehlen eine Basisklasse "ListNode" zu machen, und eine Klasse "List", die eben "ListNode" Instanzen "listen" kann. Dann leitest du deine Elemente einfach von "ListNode" ab, und gibst der Klasse die diese Elemente listen soll ein Member "List".

oder man fragt sich ganz ganz lange, ob man wirklich vererbung meint, und verlangt dann einfach die members prev und pred. und der benutzer kann wieder über den leckersten member-platz verfügen.

@ DJohn

Danke, bin ja auch ganz schön beschränkt, die "Auffangvariablen" als Pointer zu definieren, und dann keinen Speicherplatz zu reservieren. Vielen Dank, jetzt funktioniert das ganze ohne Probleme - zumindest bis 2000 Objekte (mehr als 100 dürften es allerdings nie werden. !!!! NOCHMALS DANKE !!!! Auch mal wieder ein klassisches Beispiel dafür, dass man bei Problemen erstmal die eigenen Schauklappen abnehmen, und sich den ganzen Code anschauen sollte, bevor man anderen Leuten die Zeit stiehlt .

Auch danke an die anderen, die sich die Zeit genommen haben.

@ Thomas...

Die Template Idee ist an sich ganz gut, allerdings halte ich es für meinen Lernerfolg sinnvoller erstmal den Umgang mit Objekten richtig zu beherrschen,bevor ich weiter mache.

@ Hustbear

Und wenn du schon mit "intrusive lists" arbeiten willst, dann würde ich dir empfehlen eine Basisklasse "ListNode" zu machen, und eine Klasse "List", die eben "ListNode" Instanzen "listen" kann. Dann leitest du deine Elemente einfach von "ListNode" ab, und gibst der Klasse die diese Elemente listen soll ein Member "List".

Ich dachte ich hab das schon (in Grundzügen) gemacht (Listnode <=> CWare, List <=> CWareList) ?.
Ich würde auch sehr dankbar sein, wenn du mir sagen könntest, was alles grausig ist (und was auch an der "Benamsung" so schlimm ist) - möchte mich schließlich verbessern.