C struct array + malloc

Hallo,
ich wollte mal fragen, ob der Code so passt. Laut Ausgabe sollte es passen, da ich aber mit malloc immer wieder meine Probleme habe, frag ich mal sicherheitshalber nach.
Es kann sein, dass einige includes unnötig sind (das ist nur ein Teil des Codes).

Aufgabe des Codes:
Der Code soll dynamisch ein Spielfeld erstellen (in diesem Codestück wurde die Größe mal fixiert) und dafür den Speicher anfordern. Das Spielfeld soll über ein 2D Array aufrufbar sein und einen Wert (100) sowie ein Mutex besitzen.

Frage:
Wird der Speicher richtig angefordert?

Meines Erachtens machst du alles richtig.

Edit: Wenn du's ganz richtig machen willst, müßtest du noch prüfen, ob malloc() 0 bzw. NULL zurückliefert (das tut es, wenn nicht genug Speicher verfügbar ist). Darauf verzichten aber die meisten C-Programmierer

Darauf sollte aber kein C-Programmierer verzichten.
Außerdem solltest du noch irgendwann free() aufrufen, um den Speicher wieder freizugeben.
Normalerweise passiert das zwar, wenn das Programm beendet wird, aber man muss sich da
nicht 100% drauf verlassen, dass das immer passiert. Also lieber selbst machen.

So danke für die Hinweise, ich hab den Code mal upgedated (updates sind fett). Nehme an, dass es so passt.

Nicht schlecht, aber ein paar Anmerkungen habe ich noch:

1. Prinzipiell ist an "return NULL;" in der main() nichts auszusetzen, weil NULL = 0 zum Typ int (Rückgabetyp von main()) kompatibel ist. Allerdings gibt es eine ungeschriebene Konvention, daß main() dann 0 zurückgibt, wenn alles geklappt hat, und im Fehlerfall einen von 0 verschiedenen Fehlercode. Man mag mir jetzt Pedanterie vorwerfen, aber der Vollständigkeit halber wollte ich es erwähnt haben

2. Wenn in der for-Schleife ein malloc fehlschlägt, dann returnst du, ohne die Teile des Spielfelds, die schon allokiert wurden, wieder freizugeben. Hier gilt sinngemäß das, was asdfman in Post #3 schrieb.

3. Da dein vollständiges Programm wahrscheinlich etwas länger ist als dieser Beispielcode, würde ich dir empfehlen, das Allokieren und Freigeben des Spielfelds jeweils in eine Funktion auszulagern. Auf diese Weise machst du den Programmcode übersichtlicher. (Vor dem Freigeben eines Pointers bitte auf != NULL prüfen )

Wieso da ganze nicht auf einmal allokieren und so die Speicherfragmentierung vermeiden ?
Hab schon lange nix mehr in C gemacht (aber des kann ich noch erkennen, daß der obige Code was Laufzeitverhalten und Speicherverwaltung (auch Code Wartung) betrifft schon heftig is...)

struct feld* spielfeld_Tmp;
struct feld** spielfeld;

spielfeld_Tmp = (struct feld*)malloc(sizeof(struct feld)*x*y); //also alle felder auf einmal aufm HEAP
spielfeld = (struct feld**)spielfeld_Tmp; //jetzt den Array-Zeiger einfach in einen doppel Array Zeiger casten

1. Prinzipiell ist an "return NULL;" in der main() nichts auszusetzen, weil NULL = 0 zum Typ int (Rückgabetyp von main()) kompatibel ist. Allerdings gibt es eine ungeschriebene Konvention, daß main() dann 0 zurückgibt, wenn alles geklappt hat, und im Fehlerfall einen von 0 verschiedenen Fehlercode. Man mag mir jetzt Pedanterie vorwerfen, aber der Vollständigkeit halber wollte ich es erwähnt haben

Zum Vergrößern anklicken....

NULL zurückzugeben ist grundsätzlich schlecht. Weil NULL ein Zeiger ist und nicht zwangsläufig den Wert 0
hat. Er zeigt nur auf einen garantiert ungültigen Speicherbereich. Also sollte main() einen integer (gemäß
seiner Deklaration) zurückgeben.

0 bei Erfolg oder etwas Anderes bei einem Fehler zurückzugeben ist auch nicht so ganz toll. Was ist, wenn
es eine merkwürdige Plattform gibt, bei der Anderes erwartet wird? Man sollte hier lieber EXIT_SUCCESS
oder EXIT_FAILURE benutzen. Wobei das meiner Meinung nach nicht so dramatisch ist, und ich selbst es
in der Regel auch direkt mit Zahlen mache :/

Vielleicht hat ja jemand Lust, im Standard nachzulesen, ob 0 bei Erfolg und irgendetwas Anderes bei einem
Fehler klar geht, oder nicht. Ich weiß es nicht genau und würde mich wundern, warum es oben genannte
Makros gibt, wenn die Werte eh standardisiert sind.

Hast recht - in C ist NULL tatsächlich vom Typ (void*). Ich war gedanklich bei C++, wo es einfach nur als 0 #define'd ist. Laufen würde es trotzdem, wegen impliziter Typumwandlung, ist aber nicht "sauber".

Die 0-bei-Erfolg-Konvention ist POSIX-weit üblich, ebenso unter Windows.

Also zur NULL-Diskussion zitiere ich einfach mal den (99er) Standard:

7.17 (3)

The macros are
NULL
which expands to an implementation-defined null pointer constant;

Zum Vergrößern anklicken....

und 6.3.2.3 (3)

An integer constant expression with the value 0, or such an expression cast to type
void *, is called a null pointer constant.55) If a null pointer constant is converted to a
pointer type, the resulting pointer, called a null pointer, is guaranteed to compare unequal
to a pointer to any object or function.

Zum Vergrößern anklicken....

Davon abgesehen, sollten die Subarrays natürlich auch noch alle ein free spendiert bekommen.

Außerdem frage ich mich nach dem Sinn der Mutexe. Würde mich mal ernsthaft interessieren. Weil wenn das in Richtung Multithreading und Performance gehen soll, wäre das absolut kontraproduktiv und die Performance, so wie es ist, mit Sicherheit deutlich geringer als in einem Singlethread-Programm.

So danke für euer Feedback.
Code wurde wieder etwas angepasst.

@refri89
Dein Code halt leider einen "Segmentation fault" verursacht.

@7H3 N4C3R
Wenn ich "spielfeld" freigebe, werden dann nicht auch alle Subarrays freigegeben oO? Bitte deinen 2 Absatz etwas genauer ausführen.

dankö nochmal an alle für euer Feedback

Wenn ich "spielfeld" freigebe, werden dann nicht auch alle Subarrays freigegeben oO? Bitte deinen 2 Absatz etwas genauer ausführen.

Zum Vergrößern anklicken....

Jeden einzelnen Speicherbereich, den du dir mit malloc() holst, musst du mit free() wieder freigeben.

hmm ich habs mit

probiert, da kommt dann immer die Fehlermeldung

error: cannot convert ‘feld’ to ‘void*’ for argument ‘1’ to ‘void free(void*)’

Zum Vergrößern anklicken....

Also zum Punkt mit den Mutexen:

Mein Bauchgefühl sagt mir: Viel zu feingranular. Folgendes ist nur aus Annahmen geraten.

Zuerst scheint mir erstmal das gesamte Spielfeld in einer "Aktion" als schützenswert, nicht nur ein einzelnes Feld. Zweitens würde ich spontan erwarten, dass die Feldinhalte sich oft ändern oder oft gelesen werden. Wenn dafür jedes Mal ein Mutex bemüht ist, sprengt das jede Grenze von gut und böse, da die Synchronisationsoperationen um ein vielfaches teurer sind als der eigentliche Zugriff auf den Wert.

Außerdem hat man so, wie der Speicher allokiert ist, ein False Sharing zwischen Threads. Die Daten liegen so in der selben Cacheline. D.h. wenn ein Prozessor einen Wert ändert, invalidiert das die Caches der anderen Prozessoren - selbst wenn sie sich nicht für die konkrete Änderung interessieren - da jeder das Spielfeld im Cache halten wird und dann extrem teuer (in Relation zum Cache) aus dem Hauptspeicher neu laden muss.

Mit 2 Cores/Prozessoren würde ich aus dem Bauch raus sagen, mindestens 50% langsamer (ich glaube, diese Schätzung ist noch sehr optimistisch). Und um so langsamer, je mehr Cores dazu kommen.

Also, um Mutex komm ich ned herum, das wird verlangt

Beschreibung des Progs:
2-26 Threads fahren am Feld herum und ziehen Wände hoch (tron). Über Mutex soll immer das Feld gesperrt werden, auf welches ein Thread fahren möchte.

Eine Funktion EXIT_SUCCESS zurückgeben lassen ist Quatsch. Das ist nur für den Exitstatus des ganzen
Programms gedacht.

Was das free() angeht, scheinst du nur y mal malloc() aufzurufen, dann aber x*y mal free(). Da kann doch
etwas nicht stimmen. Für jedes malloc() einmal free(). Nicht öfter. Nicht seltener.

stimmt, hab Sch... gelabert, dann aber schnell den DEVCPP
geladen und nochmal selbst (Threads auskommentiert, weil grad die Thread Lib einlinken mir zu deppert war, aber des sollt ja nicht der punkt sein)

#include <stdio.h>
#include <stdlib.h>
//#include <pthread.h>
#include <unistd.h>
#include <ctype.h>

struct feld
{
//pthread_mutex_t mutex;
int value;
};

int main()
{
int width=10, height=10, i=0, j=0;
struct feld* spielfeld;

spielfeld = (struct feld*)malloc(sizeof(struct feld)*width*height);
if(spielfeld == NULL)return EXIT_FAILURE;


for(i=0; i<height; i++)
{
for(j=0;j<width; j++)
{
//pthread_mutex_init(&( (spielfeld+i*width+j)->mutex),0);
(spielfeld+i*width+j)->value = 100;
}
}

(spielfeld+5*width+5)->value=2;
(spielfeld+9*width+9)->value=22;

printf("%d",(spielfeld+9*width+9)->value);
free(spielfeld);//gibt speicher wieder frei

return EXIT_SUCCESS;
}


Man kann nicht mehr mit dem [][] für doppelte Arrays zugreifen, das war auch der Fehler in meinem ersten Schnellschuß, denn bei feld spielfeld[][] bzw. feld **spielfeld wird ein Array dessen Einzelelemente wieder Arrays und dann dessen Einzelemente "feld-structs" sind erwartet.

Des geht vermutlich an deinem Gesuch vorbei, da du vmtl. den doppelten Array [][] Operator bevorzugt verwenden möchtest und nicht die Pointer Arithmetik, allerdings tät mich die Unübersichtlichkeit und die Speicherfragmentierung stören...brauchst aber nur ein free() für ein zusammenhängenden Speicherbereich und MemoryLeaks sind so ziemlich das Übelste was es gibt...

So scheint es zu gehen, danke an asdfman

um [][] Operator benutzen zu können kommst eben um eine extra schleife nicht rum...


#include <stdio.h>
#include <stdlib.h>
//#include <pthread.h>
#include <unistd.h>
#include <ctype.h>

struct feld
{
//pthread_mutex_t mutex;
int value;
};

int main()
{
int width=10, height=10, i=0, j=0;
struct feld* spielfeld_TMP;
struct feld** spielfeld;

spielfeld_TMP = (struct feld*)malloc(sizeof(struct feld)*width*height);
if(spielfeld_TMP == NULL)return EXIT_FAILURE;
spielfeld = (struct feld**)malloc(sizeof(struct feld *)*height);
if(spielfeld == NULL)return EXIT_FAILURE;

for(i=0; i<height; i++)
spielfeld = spielfeld_TMP+i*width;

for(i=0; i<height; i++)
{
for(j=0;j<width; j++)
{
//pthread_mutex_init(&(spielfeld[j].mutex),0);
spielfeld[j].value = 100;
}
}

spielfeld[5][5].value=2;
spielfeld[9][9].value=22;

printf("%d",spielfeld[9][9].value);
free(spielfeld);//gibt speicher wieder frei
free(spielfeld_TMP);//gibt speicher wieder frei

return EXIT_SUCCESS;
}

Mark.G. schrieb:

So scheint es zu gehen, danke an asdfman

Zum Vergrößern anklicken....

Würde empfehlen, das Programm auch mal durch Valgrind laufen zu lassen, falls du noch was
übersehen hast.