2014-02-28 5 views
5

Ich lerne gerade Valgrind und c zu verwenden, und ich habe eine ungültige free() - Ausgabe beim Versuch, Daten aus einer Struktur freizugeben. Ich glaube, das liegt daran, dass Daten nicht korrekt von der Struktur befreit werden.Hausaufgaben: Freigeben von Daten in einer Struktur

Das ist mein struct:

typedef struct song_ 
{ 
    char *artist; 
    char *title; 
    mtime *lastPlayed; 
} song; 

Und dies ist die Funktion, die es zu befreien versucht:

void songDelete(song *s) 
{ 
    //artist 
    free(s->artist) ; 
    //title 
    free(s->title) ; 
    //time 
    if(NULL != s->lastPlayed) 
     mtimeDelete(s->lastPlayed) ; 
    //song 
    free(s); 
} 

mtime und mtimeDelete einige benutzerdefinierte Variablen und Methoden, aber ich fühle mich wie Sie sind nicht relevant für meine Frage. Ich weiß, dass es falsch ist, jemanden zu bitten, meine Hausaufgaben für mich zu machen, ich möchte nur einen Schub in die richtige Richtung, wenn möglich.

+0

Was ist diese ungültige Ausgabe, die Sie bekommen? – emecas

Antwort

3

Nein, das ist definitiv der richtige Weg.

Also, wenn valgrind beschwert, ist es wahrscheinlich, weil die Werte in artist, title oder lastPlayed sind nicht wirklich gültige Zeiger.

Das ist das erste, was ich überprüfen würde.

Mit anderen Worten, stellen Sie sicher, was Sie setzen in gibt es gültige Zeiger. Erstellen Sie einfach einen Song mit:

song *AchyBreakyHeart = malloc (sizeof (song)); 

wird die Felder nicht ausfüllen (sie werden auf willkürliche Werte festgelegt). Ähnlich wird

AchyBreakyHeart->artist = "Bill Ray Cyrus"; 

wird es mit einer Zeichenfolge-Konstante anstelle eines gültigen Zeigers im Heap auffüllen.

Die ideale Sache wäre, einen „Konstruktor“ zu haben, ähnlich wie die destructor Sie zur Verfügung gestellt haben, so etwas wie:

song *songCreate (char *artist, char *title, mtime *lastPlayed) { 
    song *s = malloc (sizeof (song)); 
    if (s == NULL) return NULL; 

    s->artist = strdup (artist); 
    if (s->artist == NULL) { 
     free (s); 
     return NULL; 
    } 

    s->title = strdup (title); 
    if (s->title == NULL) { 
     free (s->artist); 
     free (s); 
     return NULL; 
    } 

    s->lastPlayed = mtimeDup (lastPlayed); 
    if (s->lastPlayed == NULL) { 
     free (s->title); 
     free (s->artist); 
     free (s); 
     return NULL; 
    } 

    return s; 
} 

Dies garantiert, dass das Objekt entweder vollständig aufgebaut oder gar nicht gebaut (dh keine Halbzustände).

Noch besser wäre es, das Konstruktor/Destruktor-Paar so anzupassen, dass NULLs in Verbindung miteinander behandelt werden, um das Paar zu vereinfachen. Zunächst wird eine leicht modifizierte destructor, wobei die einzige Änderung, dass es NULL annehmen können und ignorieren:

void songDelete (song *s) { 
    // Allow for 'songDelete (NULL)'. 

    if (s != NULL) { 
     free (s->artist); // 'free (NULL)' is valid, does nothing. 
     free (s->title); 
     if (s->lastPlayed != NULL) { 
      mtimeDelete (s->lastPlayed) ; 
     } 
     free (s); 
    } 
} 

Als nächstes wird der Konstruktor, der, anstatt zu versuchen, sich daran zu erinnern, was zugeteilt wurde, stattdessen setzt sie alle auf NULL zunächst und fordert nur die destructor, wenn etwas schief geht:

song *songCreate (char *artist, char *title, mtime *lastPlayed) { 
    // Create song, null all fields to ease destruction, 
    // then only return it if ALL allocations work. 

    song *s = malloc (sizeof (song)); 
    if (s != NULL) { 
     s->artist = s->title = s->lastPlayed = NULL; 

     s->artist = strdup (artist); 
     if (s->artist != NULL) { 
      s->title = strdup (title); 
      if (s->title != NULL) { 
       s->lastPlayed = mtimeDup (lastPlayed); 
       if (s->lastPlayed != NULL) { 
        return s; 
       } 
      } 
     } 
    } 

    // If ANY allocation failed, destruct the song and return NULL. 

    songDelete (s); 
    return NULL; 
} 
+0

Danke, ich werde auf jeden Fall überprüfen und sicherstellen, dass ich das richtig gemacht habe. – bumbleBumble

1

Ihr Code scheint korrekt zu sein.

Stellen Sie sicher, dass Ihre Struktur ordnungsgemäß initialisiert ist (mit Pointern, die auf NULL oder einen gültigen Wert festgelegt sind).

int main() { 
    song_ *s = calloc(sizeof(song_)); 
    free(s); 
    return 0; 
} 
+0

'frei (e)' ist nicht gültig. 's' ist kein Zeiger. – Asblarf

+0

@Asblarf, danke. Antwort bearbeitet. –

+0

@Asblarf Sie müssen 'free (& s)' nicht aufrufen, weil Sie 's' nicht im Heap allcotiert sind. – BlackMamba

0

Haben Sie erstellen ein Objekt/Zeiger auf Ihre Struct mit malloc (Heapzuordnung)? oder einfach song s; (Stapelzuordnung) ??

Sie können es nicht freigeben, wenn es nicht malloc'd ist, oder mit anderen Worten, Sie können eine Variable nicht auf stack freigeben, es muss auf der heap sein.

+0

Ja, es wurde definitiv mit malloc gemacht. (Oder sollte es sein und ich vermasselt. Ich werde überprüfen, ob ich immer malloc benutze.) – bumbleBumble

Verwandte Themen