[C++] Esercizio: gestione carte SIM ricaricabili e problemi con puntatori

[MegaAlchimista]

Comunque ti sei dimenticato il distruttore :-)

//in sms.h
     ~Sms()

//in sms.cpp
Sms::~Sms()
{
    delete mittente;
}

[shodan]

Comunque ti sei dimenticato il distruttore :-)

Per sua fortuna, altrimenti avrebbe un crash a ogni chiamata

[walzer91]

Si, è vero devo usarlo più spesso il distruttore grazie ad alchimista per aver risolto il problema!

[MegaAlchimista]

Originariamente inviato da shodan
Per sua fortuna, altrimenti avrebbe un crash a ogni chiamata

perchè? A me funzionava...
magari c'è qualcosa che non ho visto o che non conosco

[shodan]

Se Sms viene passato per copia o assegnato a un altro Sms il puntatore viene cancellato due volte. Il fatto che in quel main queste operazioni non siano effettuate è ininfluente per dire che c'è un grosso bug.
Per questo parlavo di malagestione del puntatore qualche post fa.

[MegaAlchimista]

Scusami tu mi parli di copia ed assegnamento ma lui non ha definito ne il costruttore di copia ne l'operatore di assegnamento. Non ho capito se il problema risiede in questo, o da qualche altra parte. Perchè e dove verrebbe cancellato due volte?
Per esempio, così su due piedi,
[a parte il fatto che farei un overloading di setmittente(sim*)]
il costruttore di copia lo definirei così

Sms::Sms(const Sms& sms)
{
    string h, f, g;
    double i;            
                
    setcosto(sms.costo);
    setdestinatario(sms.destinatario);
    setdata(sms.data);
    settesto(sms.testo);
    
    h = sms.mittente->getnumero();
    f = sms.mittente->getpin();
    g = sms.mittente->getfiscale();
    i = sms.mittente->getresiduo();
    
    setmittente(h,f,g,i);
    //oppure setmittente(sms.mittente); nel caso definissi void setmittente(sim*)
}

Cerco di immaginare un'operazione come questa

    Sim miaSim("numero", "pin", "codiceFiscale", 2.12);
    Sms sms1(miaSim, 1,"destinatario","data","testo");

    Sms sms2(sms1);

e non immagino i due punti nei quali il puntatore a Sim verrebbe cancellato.
Scusa se ti rompo ma credo di poter imparare molto :-) se ti va di rispondermi ne sarò felice

[shodan]

Perchè e dove verrebbe cancellato due volte?

Come ho detto, nel codice proposto dall'op non c'è nessuna copia e/o assegnazione per cui inserire il distruttore e relativa cancellazione del puntatore è safe.

Però
non appena venisse richiesta l'operazione di copia e/o assegnamento in quel codice, un solo puntatore sarà cancellato senza problemi quando entrambi gli oggetti usciranno di scope, l'altro punterà alla stessa zona di memoria già rilasciata e il programma s'impalla.
Non bisogna dimenticare che il costruttore di copia e l'operatore di assegnamento sono generati in automatico dal compilatore se non sono esplicitati dal programmatore. In quel caso viene effettuata una copia bit a bit di ogni singola variabile di classe. Se le variabili a loro volta prevedono costruttori di copia, essi saranno invocati in automatico, ma di un puntatore raw viene sempre e solo fatta la copia bit a bit.

Nel tuo codice prevedi un costruttore di copia e questo ti salva in questa operazione:

Sms sms2(sms1);

ma non in questa:

Sms sms2;
sms2 = sms1;

in questo caso a essere invocato è l'operatore di assegnamento non il costruttore di copia, ma tu non hai nessun operatore di assegnamento.
Quando sms2 (creato per ultimo e quindi con scope più breve) sarà distrutto, effettuerà la corretta delete del puntatore. Quando anche sms1 uscirà di scope, però, anche lui distruggerà il suo puntatore che, per quanto detto prima, punterà a una zona non valida di memoria. Di conseguenza ci sarà un crash.

La regola è: quando una classe deve controllare il lifetime di un puntatore raw contenuto al suo interno è obbligatorio scrivere costruttore di copia, operatore di assegnamento e distruttore in modo che ogni classe abbia il suo puntatore isolato dalle copie. Sempre.
Poi si può discutere se conviene usare un reference counter, perché solo una classe abbia il possesso esclusivo del puntatore (e le altre no), oppure fare una deep copy dei dati (li dipende dal design della classe).
Pensa se porti un esercizio del genere davanti al professore. Gli mostri:

Sms sms2(sms1);

e funziona. Poi lui cambia in:

Sms sms2;
sms2 = sms1;

e si pianta tutto. Non credo sia divertente.

[MegaAlchimista]

si ok allora avevo capito :-) il problema stava nel fatto che lui non li aveva proprio definiti (copia e assegnamento) e non altrove.
Il discorso che fai tu e giusto e l'avevo capito, mi era solo sorto il dubbio che ci fosse qualche altro errore in altri punti.
Comunque tagliando la testa al toro, visto che in questo periodo mi sto proprio esercitando su questi argomenti e che questa discussione mi era sembrata molto didattica , dopo pranzo avevo implementato i metodi dei quali abbiamo appena parlato, ma mentre scrivevo l'operatore di assegnamento m'è sorto un dubbio, ho trovato una soluzione che appare stabile ma non sono sicuro della sua correttezza.
In sms, nell'operatore di assegnamento per copiare l'oggetto sim ho implementato un metodo che torna una copia di mittente

Sms::Sms(const Sms &s)
{
    this->operator =(s);
}
Sms& Sms::operator =(const Sms& s)
{
    if(this==&s)
        return *this;
    costo = s.getCosto();
    destinatario = s.getDestinatario();
    data = s.getData();
    testo = s.getTesto();
    setMittente(s.getMittente());
    return *this;
}
Sim Sms::getMittente() const
{
    Sim copia = Sim(*mittente);
    return copia;
}
void Sms::setMittente(Sim newSim)
{
    mittente = new Sim(newSim);
}
/*     ovviamente ho anche ridefinito ==     */

bool Sms::operator ==(const Sms& s) const
{
    if(costo == s.getCosto() && destinatario==s.getDestinatario() && data==s.getData()
            && testo == s.getTesto() && *mittente==s.getMittente()
            )
        return true;
    return false;
}

questo è il costruttore di copia di Sim

Sim::Sim(const Sim &s)
{
    numero = s.getNumero();
    pin = s.getPin();
    fiscale = s.getFiscale();
    residuo = s.getResiduo();
}
Sim& Sim::operator=(const Sim& s)
{
    if(this==&s)
        return *this;
    numero = s.getNumero();
    pin = s.getPin();
    fiscale = s.getFiscale();
    residuo = s.getResiduo();
    return *this;
}
bool Sim::operator ==(const Sim& s) const
{
    if(numero==s.getNumero() && pin==s.getPin() && fiscale==s.getFiscale() && residuo==s.getResiduo())
        return true;
    return false;
}

è quel getMittente che mi puzza: creo un nuovo oggetto Sim, passando al costruttore di copia di sim il puntatore al puntatore(dato che prende un indirizzo), e poi faccio tornare questo.
Facendo così nel main ho messo

Sim b1("3234686493", "3454", "RETUIN91H03G273T", 2.12);
    Sms c1(b1,0.15, "092353943", "2000/1/1", "Prova");
    Sms c2(c1);
//stampa c1,c2
Sim b("111111111", "PIN", "codicef1sc4l3", 2.12);
    Sms c3(b,0.15, "000000000", "2002/2/2", "sono il terzo sms");
c1=c3;
//stampa c1,c2,c3

e funziona tutto correttamente, nessun crash, la sim di c2 rimane b1 mentre la sim di c1 diventa b.
Ma forse invece che una copia dovevo far tornare il puntatore, e poi fare un overloading di setMittente che prendesse un puntatore?
Una cosa del genere

Sim* Sms::getMittente() const
{
    return mittente;
}
void Sms::setMittente(Sim* newSim)
{
    mittente = newSim;
}

A me è sembrato più giusto copiarlo perchè ho pensato che se ci sono due puntatori che puntano alla stessa area di memoria, ed il primo che viene cancellato fa il delete, poi il secondo punta a void. Però non mi sento sicurissimo di ciò che ho scritto

[shodan]

Invece di due metodi, puoi fare direttamente:

mittente = new Sim(*mittente);

nell'operatore di assegnamento. Ti eviti tre inutili invocazioni del costruttore di copia di Sim.
Tra l'altro è preferibile definire l'operatore di assegnamento come caso speciale del costruttore di copia (l'idioma copy and swap), piuttosto che il contrario. La ragione è che si sfrutta la initialization list del costruttore di copia e si evitano i problemi con possibili eccezioni.

Forse è meglio se apri un thread apposta visto che questo era dedicato a un altro problema.

[MegaAlchimista]

No va bene così mi importava fondamentalmente sapere che fosse giusto fare

    mittente = new Sim(*mittente);

Inoltre hai ragione sulle tre invocazioni inutili.
Ora andrò a studiare la initialization list ed evenuali problemi correlati.
Grazie della dritta e buona serata!