[C++]operator+ return probleem

[Verwijderd]

Ik heb een beetje een gelijkaardig probleem zoals op [rml][ C++] returnen van class[/rml]
En ik weet niet goed hoe het op te lossen, ik heb me al rot gezocht.

Ik heb dus een class Sparse van de vorm:

code:

    int aKnopen;
    int aBogen;
    struct node {
        int knoop;
        node *next;
        struct node(int w) {knoop=w;next=NULL;}
    };
    typedef node *link;
    std::vector<link> buren;

En ik heb hiervoor een constructor, destructor en copy-constructor voorzien:

code:

    ~Sparse();
    Sparse(int aantal);
    Sparse(const Sparse &s);

Ook heb ik een operator + voorzien waarmee ik twee grafen kan optellen:

code:

const Sparse operator+(Sparse &y) const;

zoals men kan zien returned deze een "Sparse"

Bij de return (return resGraaf) wordt eerst de kopieconstructor en dan de destructor opgeroepen met als gevolg ik blijkbaar alles kwijt ben (mijn gelinkte lijst) zodat deze in het hoofdprogramma foetsjie is

Nu versta ik dat niet, want ik heb nogtans een copy-constructor geschreven dus dat zou dan toch niet mogen. Mijn vermoeden is dat er dus een fout in mijn copy-constructor zit:

code:

Sparse::Sparse(const Sparse &s) {
    aKnopen=s.aKnopen;
    aBogen=s.aBogen;
    buren=s.buren;
}

[Zoijar]

Bedoel je niet een 'const Sparse&' die je returned? 'const Sparse' heeft niet zo veel nut.

Bovendien kopieer je een vector met pointers; die pointers wijzen nu nog naar de elementen in de vector van het oude object. Oh en verder, std::list misschien iets? Nu maak je zelf een soort linked list in een vector?

[ Voor 72% gewijzigd door Zoijar op 10-06-2004 22:35 ]

[.oisyn]

C++:

const Sparse operator+(Sparse &y) const;

Maak die Sparse & eerst eens const. Bovendien hoeft de returnwaarde niet const te zijn . Maar wat doe je precies in je destructor? De lijst vrijgeven? Want dan is het natuurlijk logisch

Zoijar schreef op 10 juni 2004 @ 22:31:
Bedoel je niet een 'const Sparse&' die je returned? 'const Sparse' heeft niet zo veel nut.

Een const reference teruggeven vanuit de operator+ gaat een beetje moeilijk, je moet namelijk een nieuw object teruggeven

Bovendien kopieer je een vector met pointers; die pointers wijzen nu nog naar de elementen in de vector van het oude object.

Nou ja dat weet je nog niet (hij heeft nog niet de volledige code gegeven), maar ik denk dat dat idd ook het probleem is

[ Voor 55% gewijzigd door .oisyn op 10-06-2004 22:38 ]

[Verwijderd]

In de destructor wordt inderdaad de volledige lijst vrijgegeven (maar dat moet toch, zo hoort het toch?):

C++:

    link t;
    for(int i=0;i<aKnopen;i++) {
        while(buren[i]!=NULL) {
            t=buren[i]->next;
            delete buren[i];
            buren[i]=t;
        }
    }

En de operator+ heb ik nu als volgt gedefinieerd:

C++:

Sparse Sparse::operator+(const Sparse &y) const;

Bovendien kopieer je een vector met pointers; die pointers wijzen nu nog naar de elementen in de vector van het oude object. Oh en verder, std::list misschien iets? Nu maak je zelf een soort linked list in een vector?

Wat zou dan de juiste copy-constr zijn (moet ik die knopen in die vector dan ook dynamisch aanmaken wss wel maar zou mijn programma dan wel werken)? std::list zou ik kunnen gebruiken maar da's niet echt de bedoeling, ik wil dit zo oplossen kwestie van iets bij te leren.

[ Voor 79% gewijzigd door Verwijderd op 10-06-2004 23:16 ]

[.oisyn]

Verwijderd schreef op 10 juni 2004 @ 23:09:
In de destructor wordt inderdaad de volledige lijst vrijgegeven (maar dat moet toch, zo hoort het toch?):

C++:

    link t;
    for(int i=0;i<aKnopen;i++) {
        while(buren[i]!=NULL) {
            t=buren[i]->next;
            delete buren[i];
            buren[i]=t;
        }
    }

Dan is het toch logisch? Je maakt een Sparse aan, die intern een gelinkte lijst van iets alloceert. Dan maak je een andere Sparse aan, dat een kopie is van de eerste. Beide Sparse objecten wijzen naar dezelfde lijst. Als je dan één van die twee weer destruct, wordt de lijst vrijgegeven, en wijzen de pointers van de andere Sparse naar al vrijgegeven geheugen.

Dus je zult de hele lijst moeten dupliceren door nieuwe nodes te alloceren in de copyconstructor.

[Verwijderd]

Dan is het toch logisch? Je maakt een Sparse aan, die intern een gelinkte lijst van iets alloceert. Dan maak je een andere Sparse aan, dat een kopie is van de eerste. Beide Sparse objecten wijzen naar dezelfde lijst. Als je dan één van die twee weer destruct, wordt de lijst vrijgegeven, en wijzen de pointers van de andere Sparse naar al vrijgegeven geheugen.

Dus je zult de hele lijst moeten dupliceren door nieuwe nodes te alloceren in de copyconstructor.

Na een woelige nacht dacht ik ook al zoiets, ik ga es die copy-constructor herschrijven.
Maar ik kom er gelijk niet goed uit hoe ik die copy-constructor dan het beste schrijf. Ik heb zoiets:

C++:

Sparse::Sparse(const Sparse &s) {
    aKnopen=s.aKnopen;
    aBogen=s.aBogen;
    buren=s.buren;
    link t,head=NULL,temp;
    for(int i=0;i<aKnopen;i++) {
        t=buren[i];
        while(t!=NULL) {

        }
        buren[i]=head;
    }
}

Nu moet ik in die while(t!=NULL) die nieuwe nodes aanmaken. Hierbij wou ik via een temp link telkens de nodes aanmaken in de hoop dat als ik head in het begin gelijkstelde aan de temp dat die head zou meeveranderen zodat ik buren[i] op het einde gelijk kan stellen aan die head.
Echter die head veranderd niet mee?
Ik moet dus in feite gewoon in die copy-constructor een geschakelde lijst creëren met nieuwe nodes en die aan buren[i] hangen. Dat lukt nu blijkbaar niet (op de duur zie je scheel op al die code lol) maar ik ga toch nog ff verderproberen.
edit na wat herbronnen:

C++:

Sparse::Sparse(const Sparse &s) {
    aKnopen=s.aKnopen;
    aBogen=s.aBogen;
    buren=s.buren;
    link t,head=NULL;
    for(int i=0;i<aKnopen;i++) {
        t=buren[i];
        while(t!=NULL) {
            head=new node(t->knoop,head);
            t=t->next;
            }
        buren[i]=head;
        head=NULL;
    }
}

Ook was het noodzakelijk de header van de Sparse aan te passen zodat er ook een constructor bestond van de vorm

C++:

node(Item waarde, node* next)

Dank voor alle tips ze hebben mij veel geholpen en mij veel inzicht bijgegeven!!!

[ Voor 70% gewijzigd door Verwijderd op 11-06-2004 08:50 ]

[MSalters]

Waarom niet gewoon een std::list? Dan hoef je die logica niet zelf te dupliceren.

[Verwijderd]

Ik heb wel nog een ander vraag, waarvan ik het probleem heb opgelost maar ik snap niet goed waarom. Wederom betreft het die operator+
we hebben deze nu met als prototype:

C++:

Sparse Sparse::operator+(const Sparse &y) const

Hoe komt het nu als ik hierin een nieuwe Sparse aanmaak als volgt:

C++:

Sparse GraafRES(aantal);

en vervolgens ergens in het programma iets doe van de vorm:

C++:

GraafRES=y;

Dat die y wordt vernietigd bij de return van mijn operator+ terwijl als ik het volgende doe:

C++:

Sparse GraafRES=y;

die y niet vernietigd wordt en er dus geen problemen zijn op het einde van mijn programma waar die y nog eens vernietigd wordt.

Ik heb dit nu opgelost door de laatste constructie te gebruiken maar stel dat ik dit niet zou kunnen om één of andere reden, hoe kan ik dan zorgen dat bij de eerste constructie toch nog de y behouden blijft? Ik dacht juist dat die "const Sparse &y" daarvoor moest zorgen?

Waarom niet gewoon een std::list? Dan hoef je die logica niet zelf te dupliceren.

Omdat ik dan gans diene boel niet leer hé nu heb ik iets bijgeleerd, met STL te implementeren niet echt é want dan moet je al dat geheugenbeheer niet zelf doen.

[ Voor 13% gewijzigd door Verwijderd op 11-06-2004 09:15 ]

[farlane]

Verwijderd schreef op 11 juni 2004 @ 09:14:

Ik heb dit nu opgelost door de laatste constructie te gebruiken maar stel dat ik dit niet zou kunnen om één of andere reden, hoe kan ik dan zorgen dat bij de eerste constructie toch nog de y behouden blijft? Ik dacht juist dat die "const Sparse &y" daarvoor moest zorgen?

[...]

Ik zie in je voorbeelden niet waaro, operator+ iets met die constructie te maken zou hebben. Het verschil is wel dat de ene een operator= gebruikt ( vb1 ), en de andere een copy constructor ( vb 2 )

Het probleem is dus waarschijnlijk dat je geen of geen correcte operator= hebt gedefinieerd.( Normaal gesproken heb je die altijd nodig, als je ook een copy constructor nodig hebt )

NB VB2 lijkt ook een operator= geval, maar is het niet omdat het tegelijk ook een declaratie betreft. Je zou ook kunnen schrijven :

C++:

Sparse GraafRES( y );

[ Voor 12% gewijzigd door farlane op 11-06-2004 12:07 ]

[Verwijderd]

Het probleem is dus waarschijnlijk dat je geen of geen correcte operator= hebt gedefinieerd.( Normaal gesproken heb je die altijd nodig, als je ook een copy constructor nodig hebt )

Ah oké dat heb ik inderdaad niet gedaan, ik geloof dat er nu heel veel franken vallen hiero

[.oisyn]

Een quick'n'dirty manier voor als je toch al een goede copyconstructor en destructor hebt geimplementeerd:

C++:

Sparse & Sparse::operator = (const Sparse & s)
{
    this->~Sparse ();       // call destructor
    new (this) Sparse (s);  // call copy-ctor with placement new
}

[ Voor 13% gewijzigd door .oisyn op 11-06-2004 14:08 ]

[Soultaker]

Als je die ranzige methode gaat toepassen mag je er wel voor zorgen dat je destructor private is, anders krijg je grote problemen wanneer iemand nietsvermoedend klasse van Sparse probeert af te leiden. Je assignment operator past namelijk het run-time type van het object aan (dat is na deze functie altijd Sparse) en dat hoort een assignment operator normaalgesproken niet te doen.

Om dit effect ongedaan te maken moet een afgeleide klasse ofwel zelf alsnog de hele operator= opnieuw (en nu goed) implementeren (terwijl dat normaal alleen voor de nieuwe velden hoeft en je voor de rest de operator= van je basisklasse kunt aanroepen), ofwel van dezelfde hack gebruik maken. Geen van beide is echt een geslaagde optie, wat mij betreft.

Ik zou deze methode dus eigenlijk nooit willen aanraden, maar hooguit voor klassen die effectief 'sealed' zijn en waarvan geen andere klassen af te leiden zijn.

[.oisyn]

Soultaker schreef op 11 juni 2004 @ 14:50:
Als je die ranzige methode gaat toepassen mag je er wel voor zorgen dat je destructor private is

operator = is sowieso niet inheritable. Bovendien heb je niets aan een private destructor, dat zou betekenen dat je alleen nog maar instanties op de heap aan mag maken

Maar verder ben ik het er mee eens dat het een quick'n'dirty manier was, zoals ik in mijn vorige post al zei

[ Voor 8% gewijzigd door .oisyn op 11-06-2004 14:57 ]

[Soultaker]

.oisyn schreef op 11 juni 2004 @ 14:54:
operator = is sowieso niet inheritable, dus als er een afgeleide is dan zal deze operator = nooit worden aangeroepen.

Op zichzelf klopt dat wel, maar afhankelijk van de context kan 'ie best aangeroepen worden; bijvoorbeeld:

C++:

void foo(Base &base) {
    base = base;
}

int main()  {
    Derived derived;
    foo(derived);
    // oops: derived is nu een Base object geworden!
    foo.fooMethod(); // als dit crasht heb je geluk gehad
}

Nu geef ik zo toe dat die functie foo ook een nogal twijfelachtige implementatie heeft (zelfs als Base::operator= wel 'goed' gedefinieerd is zou het gedrag van die functie wel eens problemen op kunnn leveren) maar dat is geen reden om dan maar allerlei hacks in je operator= te gebruiken.

Bovendien heb je niets aan een private destructor, dat zou betekenen dat je alleen nog maar instanties op de heap aan mag maken

Uhm, tja... dat klopt. Hoe moet je dat dan doen?

Maar verder ben ik het er mee eens dat het een quick'n'dirty manier was, zoals ik in mijn vorige post al zei

Suggereer dan liever om de operator= correct te implementeren en de copy constructor die te laten gebruiken, in plaats van omgekeerd:

C++:

Foo(const Foo &that) {
    *this = that;
}

Dan heb je minstens net zoveel werk bespaart maar wel een heel wat veiligere oplossing.

[ Voor 3% gewijzigd door Soultaker op 11-06-2004 15:03 ]

[.oisyn]

Soultaker schreef op 11 juni 2004 @ 15:03:
[...]

Op zichzelf klopt dat wel, maar afhankelijk van de context kan 'ie best aangeroepen worden; bijvoorbeeld:

C++:

void foo(Base &base) {
    base = base;
}

int main()  {
    Derived derived;
    foo(derived);
    // oops: derived is nu een Base object geworden!
    foo.fooMethod(); // als dit crasht heb je geluk gehad
}

Nu geef ik zo toe dat die functie foo ook een nogal twijfelachtige implementatie heeft (zelfs als Base::operator= wel 'goed' gedefinieerd is zou het gedrag van die functie wel eens problemen op kunnn leveren)

Mja, je assignt een Base aan een Base, dus het resultaat zou ook een Base moeten zijn. Operator = en inherited classes gaan niet echt goed samen. Je zou 'm virtual kunnen maken, maar dat betekent dat je in Derived een operator = moet implementeren die een Base & accepteert, wat natuurlijk gewoon niet logisch is. Ergo, als je een Base & hebt en je assignt er een andere Base & aan, dan zou het resultaat gewoon een Base moeten zijn, ook al waren beide instanties eigenlijk Derived's

Daarnaast gaat je argument dat het runtime type veranderd door de copyconstructor in het geval van Sparse niet op; het heeft geen vtable, dus er is ook geen runtime type.

Uhm, tja... dat klopt. Hoe moet je dat dan doen?

Idd, niet dus... Java's final zou een oplossing kunnen bieden, maar dat heb je helaas niet in C++. Aan de andere kant heb je niet zo heel veel aan het overerven van een klasse zonder virtual functions, dus waarom zou je überhaupt?

Suggereer dan liever om de operator= correct te implementeren en de copy constructor die te laten gebruiken, in plaats van omgekeerd:

C++:

Foo(const Foo &that) {
    *this = that;
}

Dan heb je minstens net zoveel werk bespaart maar wel een heel wat veiligere oplossing.

Dat is een imho net zo ranzige constructie. Als je het op die manier doet zul je sowieso opruimcode in de operator = moeten zetten om de vorige lijst vrij te geven alvorens je een nieuwe assignt. En aangezien je de operator = aanroept vanuit de constructor, kan het dus zijn dat je sommige variabelen nog niet hebt geinitialiseerd, dus dan zul je eerst een valide leeg object moeten construeren alvorens je de operator = aanroept.

Als je echt coderedundancy wilt voorkomen kun je het beste denk ik gewoon een private void assign (const T &) en een void cleanUp () maken die je vanuit de copy-ctor, dtor en operator = aan kunt roepen

[Soultaker]

.oisyn schreef op 11 juni 2004 @ 15:33:
Dat is een imho net zo ranzige constructie. Als je het op die manier doet zul je sowieso opruimcode in de operator = moeten zetten om de vorige lijst vrij te geven alvorens je een nieuwe assignt. En aangezien je de operator = aanroept vanuit de constructor, kan het dus zijn dat je sommige variabelen nog niet hebt geinitialiseerd, dus dan zul je eerst een valide leeg object moeten construeren alvorens je de operator = aanroept.

Ik dacht dat in de constructor alle members al wel default constructed zouden zijn?

[.oisyn]

Nou stel je hebt gewoon een losse pointer die ergens naar wijst, en die je delete voordat je een nieuwe assignment doet. Die pointer zal niet op 0 geinitialiseerd worden, waardoor je heap corruption krijgt als je 'm in operator = wil deleten.

Maar goed, wie gebruikt tegenwoordig nog gewone pointers