コード分析
これを少しずつ分析してみましょう。
int function2(struct Person *list) {
Iserniが彼の回答で指摘したように、この定義は関数の呼び出しと矛盾しています。私は、既存のコードに対する彼の修正に概ね同意します (ただし、すぐに修正することをお勧めします)。
int function2(struct Person *list, int *index)
struct Person *prsn = malloc(sizeof(struct Person));
// !Why do we sometimes cast the malloc or not?
// I sometimes get errors when I do, sometimes when I don't,
// while the surrounding code is pretty much the same.
それは、C と C++ のどちらを使用しているかによって異なります。C++ では、からの戻り値をキャストする必要がありmalloc()ます (使用する場合malloc()、通常は C++ では使用しないでください)。C では、キャストはオプションです。キャストを省略するとエラーが明らかになり、キャストを挿入すると隠れるという考え方があります。私はそれに同意しません。それmalloc()は経由<stdlib.h>で宣言されるべきであり、スコープに宣言がない場合、コンパイラは警告する必要があり、スコープに宣言がある場合、キャストは罪を隠すことができないと思います。もう 1 つの考えられる問題は、ポインタを非ポインタに割り当てることです。これは、コンパイラーも文句を言うべき間違いです。
assert(prsn != NULL);
これは通常、メモリ割り当てエラーを処理する賢明な長期的な方法とは見なされません。
// User input code goes here ...
// Now to save the Person created
strcpy(prsn->name, nameInput);
prsn->age = ageInput;
list[(*index)++] = *prsn;
// !Why use the dereferencing *prsn here?
なぜなら：
listですstruct Person *。- したがって
list[i]、struct Person（と綴らiれていますが(*index)++）です。
- したがって、それに a を割り当てる必要があります
struct Person。
prsnですstruct Person *。- したがって
*prsn、 もstruct Personです。
- したがって、割り当ては「正しい」です。
- それはまたあなたに漏れを与えます。
list[i]のコンテンツをのコンテンツで上書きしました*prsn。prsnポインターをどこにも保存していません。- そのため、関数から戻るときにメモリ リークが発生します。
これを修正するために必要な手術は無視できません。
int function2(struct Person **item)
...
*item = prsn;
呼び出しを修正する必要があります。解剖するときに戻ってきますmain()。
// why not directly prsn? Or is that saving the memory address and not very useful.
}
あなたの関数は を返すように宣言されてintいますが、 no を示していますreturn。void値を返さない場合は、特にコードのように戻り値を無視する場合は、関数を として宣言しますmain()。
あなたの最後のコメントは、ほとんど上記の議論でカバーされています。メモリアドレスを保存することは、リークを止めるために重要であるため、非常に便利です。
そして、これが私の主な機能です：
int main(int argc, char *argv[]) {
struct Person personList[MAX_SIZE];
int index;
初期化されていない変数を使用することは悪いニュースです。せいぜい誤ってゼロになっただけです。ランダムな値を配列インデックスとして使用する余裕はありません。明示的に初期化します。また、構造体の配列ではなく、ポインターの配列が必要になります。
struct Person *personList[MAX_SIZE];
int index = 0;
...other code...
function2(personList, &index);
改訂された機能では：
function2(&personList[index++]);
これは望ましいことです。function2()これは、配列について知る必要がないことを意味します。割り当てられたメモリ ポインターを割り当てる必要があるポインターのアドレスを渡すだけです。main()これにより、関数との間の結合が減少しfunction2()、コードが全体的に単純になります。
// Before closing, I want to free any mallocs I have done here. free()
だからあなたは書く：
for (int i = 0; i < index; i++)
free(personList[i]);
これにより、割り当てられたすべてのメモリが解放されます。
return 0;
}
main()C99 は 100% 必要ではないと言っていますが、私は の最後に明示的な return があるのが好きです。
十分な警告を有効にしてコンパイルしていることを確認してください。GCC を使用している場合はgcc -Wall、最小レベルのコンパイル警告を実行する必要があります (実行時にコードに警告が表示されないようにする必要があります)。私はより厳しい警告で実行します:gcc -std=c99 -Wall -Wextra -Wmissing-prototypes -Wold-style-definition -Wstrict-prototypes -Wshadow通常. いくつかの警告を得るには、含める-O3(またはある程度の最適化を行う) 必要があります。プロトタイプに関するものは、私が使用している古いコードベースについてのパラノイアを反映していますが、そのコードベースにはまだ K&R 関数定義が含まれています。
コメントへの回答
詳細を調べて試してみるときの最初の質問: 構造体の配列とポインターの配列の間にメモリへの影響はありますか?
はい、しかしそれはあなたが考えていることではないかもしれません。構造体の配列を使用する場合、構造体に動的にメモリを割り当てる必要はありません。これは、コンパイラが既に構造体を割り当てているためです。演習の目的はポインターと を使用するmalloc()ことなので、ポインターを使用する方が適切です。スペースに関しては、ポインタの配列で使用される合計メモリがわずかに多くなります (ただし、メモリ リークは少なくなります)。
ポインターの配列を使用するようにコードを変更しようとしています。しかし、現在function2(personList, &index);call を使用するとfunction2、次の警告が表示されます: incompatible pointer types passing 'struct Person *[512]' to parameter of type 'struct Person *'. 詳細を説明するために、主な質問に追加のコードを記述しても問題ありませんか? 注意として、プログラムが一時的に関数から関数にデータをコピーしないように、変数をできるだけ参照するようにしています。
すべての変更を行っていない場合、コンパイラは正しいです。2 つの引数を使用するコードは、1 つの引数を使用するコードよりも多くのデータを関数間でコピーします。
バージョン 1
次のプログラムは、提案された単一引数を使用して、関数 とfunction2()の間の結合を減らし、両方を単純化します。main()function2()
このコードは、コマンド ラインを使用して、Mac OS X 10.7.5 上の GCC 4.7.1 で警告なしでコンパイルされます。
gcc -O3 -g -std=c99 -Wall -Wextra -Wmissing-prototypes -Wstrict-prototypes \
-Wold-style-definition mem.c -o mem
で実行するvalgrindと、メモリリークは発生しません。
#include <assert.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
enum { MAX_INPUT = 28 };
enum { MAX_SIZE = 3 };
struct Person
{
char name[MAX_INPUT];
int age;
};
static void function2(struct Person **list)
{
struct Person *prsn = malloc(sizeof(struct Person));
assert(prsn != NULL);
char *nameInput = "This is my name";
int ageInput = 29; // Again!
strcpy(prsn->name, nameInput);
prsn->age = ageInput;
*list = prsn;
}
int main(void)
{
struct Person *personList[MAX_SIZE];
int index = 0;
function2(&personList[index++]);
function2(&personList[index++]);
function2(&personList[index++]);
for (int i = 0; i < index; i++)
free(personList[i]);
return 0;
}
バージョン 2
これは の引数が 2 つのバージョンを保持し、それ自体で行うべきfunction2()カウントを行います。main()このプログラムはまた、 の下できれいにコンパイルされ、きれいに実行されvalgrindます。
#include <assert.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
enum { MAX_INPUT = 28 };
enum { MAX_SIZE = 3 };
struct Person
{
char name[MAX_INPUT];
int age;
};
static void function2(struct Person **list, int *index)
{
struct Person *prsn = malloc(sizeof(struct Person));
assert(prsn != NULL);
char *nameInput = "This is my name";
int ageInput = 29; // Again!
strcpy(prsn->name, nameInput);
prsn->age = ageInput;
list[(*index)++] = prsn;
}
int main(void)
{
struct Person *personList[MAX_SIZE];
int index = 0;
function2(personList, &index);
function2(personList, &index);
function2(personList, &index);
for (int i = 0; i < index; i++)
free(personList[i]);
return 0;
}