プログラマ専用SNS ミクプラ

（今度は前スレでやったものを）ソートして出力したいです。
ファイルの中身は
3.5 27 jiro
2.7 24 taro
3.7 30 saburo
です
ソート方法は、ソート前のリスト：L，ソート後のリスト：S
1. Lが空なら終了
2. Lの中の最大要素Mを探索
3. MをLから削除
4. MをSに挿入（挿入はSの先頭に）
5. 1. へ
というかんじです。
今回は結構自分で考えてやってみたのですが、出力ファイルに何も表示されません。。。
どなたか力をかしてくださいお願いしますm(__)m
下にコードをはります。不明な点などあれば言ってください。

コード:

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
typedef struct school_record SRec;
struct school_record {
    float gpa;
    int credit;
    char name[200];
    SRec *next;
};
SRec head;                					 // head.next がリストの先頭要素を指す

SRec* listsort(SRec *node,int (*func)(const void *,const void *))
{
    SRec top;        						//top.nextがソート後のリストの先頭要素へのポインタ
    SRec *save1,*save2,*p,*list,*last;    		    //topの末尾がlast
											//listはソート後のmalloc
    top.next=NULL;
    node=top.next;
    last=&top;
    while(p->next!=NULL)
    {
        p=&head;
        list = (SRec *) malloc(sizeof(SRec));
        list->next=NULL;
        if(list==NULL)
        {
            fprintf(stderr,"メモリ不足");
            return NULL;
        }
        while(p->next!=NULL)
        {
        	if(func(p->next,p)==1)        //最大のものを検索
        	{
        		save1=p;                   //削除するために最大のものの１つ手前をsave1として覚える
        		save2=p->next;			  //最大のものをsave2
        	p=p->next;
        	}
        }
        save1->next=save1->next->next;      //リストの削除
        list=save2;
        list->next=top.next;
        top.next=list;

    }
    free(list);
    return top.next;

}

int compare_float(const void * a, const void * b)
{
    SRec* tmp1 = (SRec*) a;
    SRec* tmp2 = (SRec*) b;
    if (tmp1->gpa == tmp2->gpa)
        return 0;
    else if (tmp1->gpa > tmp2->gpa)
        return 1;
    else
        return -1;
}
int compare_int(const void* a, const void* b)
{
    SRec* tmp1 = (SRec*) a;
    SRec* tmp2 = (SRec*) b;
    if (tmp1->credit == tmp2->credit)
        return 0;
    else if (tmp1->credit > tmp2->credit)
        return 1;
    else
        return -1;
}
int compare_char(const void * a, const void * b)
{
    SRec* tmp1 = (SRec*) a;
    SRec* tmp2 = (SRec*) b;
    if (strcmp(tmp1->name, tmp2->name) == 0)
        return 0;
    else if (strcmp(tmp1->name, tmp2->name) == 1)
        return 1;
    else
        return -1;
}
SRec* Input(FILE* fpi)
{
    SRec *node;
    SRec *tail;
    head.next = NULL;/*これが NULL だとリストにはまだ 1つもノードがないことを表す。
                       これが NULL でないとリストに最低 1つはノードがあることになる。
                       実際にはまだリストは空っぽだから NULL する。*/
    tail = &head;
    while (1)
    {
        node = (SRec *) malloc(sizeof(SRec));//これは配列ではない
        if (node == NULL)
        {
            fprintf(stderr,"メモリ不足");
            return NULL;
        }
        if (fscanf(fpi, "%f%d%s", &node->gpa, &node->credit, node->name) != 3)
        {
            break;  // 読み込みに失敗したら EOF (feof は使わないほうがよい)
        }
        node->next = NULL;  // この node を末尾要素とする
        for (tail = &head; tail->next != NULL; tail = tail->next) ;// 現在のリストの末尾要素を探す
        tail->next = node;  // 見つかった末尾要素に node を追加する。
    }
    free(node);
    return head.next;  // リストの先頭要素へのポインタを返す。
}
void Output(SRec *node, FILE* fpo)
{
    for (; node != NULL; node = node->next)
    {
        fprintf(fpo,"%3.1f %d %s\n", node->gpa, node->credit, node->name);
    }
}
int main(int argc, char *argv[])
{
    SRec* node;
    FILE *fpi, *fpo;
    int (*func)(const void*,const void*);
    if (argc != 4)
    {
        fprintf(stderr, "Illegal number of argument.\n");
        return (-1);
    }
    if ((fpi = fopen(argv[2], "r")) == NULL)
    {
        fprintf(stderr, "Can't open input file <%s>.\n", argv[2]);
        return (-1);
    }
    if ((fpo = fopen(argv[3], "w")) == NULL)
    {
        fprintf(stderr, "Can't open output file <%s>.\n", argv[3]);
        return (-1);
    }
    node = Input(fpi);
    if (strcmp(argv[1], "gpa") == 0)
        func=compare_float;//&はどちらでもいい
    else if (strcmp(argv[1], "credit") == 0)
        func=compare_int;
    else if (strcmp(argv[1], "name") == 0)
        func=compare_char;
    listsort(node,func);
    if (node == NULL) return 1;
    Output(node, fpo);
    fclose(fpi);
    fclose(fpo);
    return 0;
}

れおさんが書きました:
コード:
typedef struct school_record SRec;

せっかくこのようにtypedefしたので、

れおさんが書きました:
コード:
struct school_record {

コード:

SRec {

と書けばいいのではないか、と思います。細かい話ですけれど。

れおさんが書きました:
コード:
SRec* listsort(SRec *node,int (*func)(const void *,const void *))

戻り値をどこでも使っていないように見えます。戻り値は必要ですか？

れおさんが書きました:

コード:

        list = (SRec *) malloc(sizeof(SRec));
        list->next=NULL;
        if(list==NULL)

malloc()の「直後で」NULLかどうかを判定する必要があるのではないか、と思います。

れおさんが書きました:
コード:
    else if (strcmp(tmp1->name, tmp2->name) == 1)

strcmp()の戻り値が「ちょうど1」であることを期待しない方がいいと思います。
正の値ではあるかもしれませんけれど。

れおさんが書きました:

コード:

SRec* Input(FILE* fpi)
{
    head.next = NULL;/*これが NULL だとリストにはまだ 1つもノードがないことを表す。
    return head.next;  // リストの先頭要素へのポインタを返す。

head.nextはどこで書き換えているのでしょうか。NULLのままreturnしているように見えます。
私の勘違いならばいいのですけれど。
もし、私の勘違いでなければ、

れおさんが書きました:
コード:
    node = Input(fpi);

この戻り値はNULLのままなので、

れおさんが書きました:
コード:
    listsort(node,func);
    if (node == NULL) return 1;
    Output(node, fpo);

Output関数を通らないように見えます。

適宜printf()を入れるなどして、戻り値が適切な値であるかどうかを
チェックされる方がよいと思います。

れおさんが書きました:

コード:

SRec* listsort(SRec *node,int (*func)(const void *,const void *))
{
    top.next=NULL;
    node=top.next;

もしかしたら、ここより後でnodeの値を書き換えていない、ということに問題があるのかもしれません。
まあ、いろいろ確認してみてください。

れおさんが書きました:
コード:
    while(p->next!=NULL)

p は初期化されていないゴミです。

コード:

while (head.next != NULL)

でしょう。

れおさんが書きました:

コード:

        list = (SRec *) malloc(sizeof(SRec));
        list->next=NULL;
        if(list==NULL)
        {
            fprintf(stderr,"メモリ不足");
            return NULL;
        }

head（リスト L）からノードを削除して top（リスト S）に追加することを繰り返すだけですから、
新しいノードは不要です。

れおさんが書きました:

コード:

            if(func(p->next,p)==1)        //最大のものを検索

while ループの最初の p は &head です。
head は .next がリストの先頭を指しますが、.gpa、.credit、.name にはデータを持っていません。

れおさんが書きました:

コード:

                save1=p;                   //削除するために最大のものの１つ手前をsave1として覚える
                save2=p->next;            //最大のものをsave2

リストの先頭が最大の場合、if の中に入らないので save1 と save2 は設定されません。

れおさんが書きました:
コード:
            p=p->next;

これが if の中にあるので、p の値が更新されない場合があり、無限ループになります。

れおさんが書きました:
コード:
        list=save2;
        list->next=top.next;
        top.next=list;

list を save2 で上書きしているので malloc で確保した領域を指すポインタがなくなります。
free(list) もできなくなります。list は不要ですから

コード:

        save2->next=top.next;
        top.next=save2;

と書くべきでしょう。

れおさんが書きました:
コード:
    free(list);

不要

れおさんが書きました:
コード:
    listsort(node,func);
    if (node == NULL) return 1;

listsort() がソートされたリストの先頭を返すので、それを受け取らないといけません。

コード:

    node = listsort(node,func);

if は Input() の直後に行うものです。

とりあえず、上記のことをすべて修正すれば動くでしょうが、他にも変なところがたくさんあります。

box さんが書きました:
コード:
SRec {
と書けばいいのではないか、と思います。細かい話ですけれど。

typedef は #define とは違って、それはできません。

れおさんが書きました:
コード:
SRec* listsort(SRec *node,int (*func)(const void *,const void *))
戻り値をどこでも使っていないように見えます。戻り値は必要ですか？

ソート済みリストの先頭を指すポインタは node には返せないので、戻り値は必要です。
戻り値を使っていないことが問題です。

れおさんが書きました:
コード:
SRec* Input(FILE* fpi)
{
    head.next = NULL;/*これが NULL だとリストにはまだ 1つもノードがないことを表す。
    return head.next;  // リストの先頭要素へのポインタを返す。
head.nextはどこで書き換えているのでしょうか。NULLのままreturnしているように見えます。

コード:

    tail = &head;
            :
        tail->next = node;  // 見つかった末尾要素に node を追加する。

ここで書き換えています。

ん～＞＜
動作が停止してしまいました・・・

コード:

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
typedef struct school_record SRec;
struct school_record {
    float gpa;
    int credit;
    char name[200];
    SRec *next;
};
SRec head;                					 // head.next がリストの先頭要素を指す

SRec* listsort(SRec *node,int (*func)(const void *,const void *))
{
    SRec top;        						//top.nextがソート後のリストの先頭要素へのポインタ
    SRec *save1=0,*save2=0,*p=0;
    top.next=NULL;
    while(head.next!=NULL)
    {
        p=&head;

        while(p->next!=NULL)
        {
            if(func(p->next,p->next->next))
            {
        		save1=p;                   //削除するために最大のものの１つ手前をsave1として覚える
        		save2=p->next;			  //最大のものをsave2
        	}
        	p=p->next;
        }
        save1->next=save1->next->next;      //リストの削除
        save2->next=top.next;
        top.next=save2;
    }
    return top.next;
}

int compare_float(const void * a, const void * b)
{
    SRec* tmp1 = (SRec*) a;
    SRec* tmp2 = (SRec*) b;
    if (tmp1->gpa == tmp2->gpa)
        return 0;
    else if (tmp1->gpa > tmp2->gpa)
        return 1;
    else
        return -1;
}
int compare_int(const void* a, const void* b)
{
    SRec* tmp1 = (SRec*) a;
    SRec* tmp2 = (SRec*) b;
    if (tmp1->credit == tmp2->credit)
        return 0;
    else if (tmp1->credit > tmp2->credit)
        return 1;
    else
        return -1;
}
int compare_char(const void * a, const void * b)
{
    SRec* tmp1 = (SRec*) a;
    SRec* tmp2 = (SRec*) b;
    if (strcmp(tmp1->name, tmp2->name) == 0)
        return 0;
    else if (strcmp(tmp1->name, tmp2->name) == 1)
        return 1;
    else
        return -1;
}
SRec* Input(FILE* fpi)
{
    SRec *node=0;
    SRec *tail=0;
    head.next = NULL;/*これが NULL だとリストにはまだ 1つもノードがないことを表す。
                       これが NULL でないとリストに最低 1つはノードがあることになる。
                       実際にはまだリストは空っぽだから NULL する。*/
    tail = &head;
    while (1)
    {
        node = (SRec *) malloc(sizeof(SRec));//これは配列ではない
        if (node == NULL)
        {
            fprintf(stderr,"メモリ不足");
            return NULL;
        }
        if (fscanf(fpi, "%f%d%s", &node->gpa, &node->credit, node->name) != 3)
        {
            break;  // 読み込みに失敗したら EOF (feof は使わないほうがよい)
        }
        node->next = NULL;  // この node を末尾要素とする
        for (tail = &head; tail->next != NULL; tail = tail->next) ;// 現在のリストの末尾要素を探す
        tail->next = node;  // 見つかった末尾要素に node を追加する。
    }
    free(node);
    return head.next;  // リストの先頭要素へのポインタを返す。
}
void Output(SRec *node, FILE* fpo)
{
    for (; node != NULL; node = node->next)
    {
        fprintf(fpo,"%3.1f %d %s\n", node->gpa, node->credit, node->name);
    }
}
int main(int argc, char *argv[])
{
    SRec *node=0;
    FILE *fpi, *fpo;
    int (*func)(const void*,const void*);
    if (argc != 4)
    {
        fprintf(stderr, "Illegal number of argument.\n");
        return (-1);
    }
    if ((fpi = fopen(argv[2], "r")) == NULL)
    {
        fprintf(stderr, "Can't open input file <%s>.\n", argv[2]);
        return (-1);
    }
    if ((fpo = fopen(argv[3], "w")) == NULL)
    {
        fprintf(stderr, "Can't open output file <%s>.\n", argv[3]);
        return (-1);
    }
    node = Input(fpi);
    if (node == NULL) return 1;
    if (strcmp(argv[1], "gpa") == 0)
        func=compare_float;//&はどちらでもいい
    else if (strcmp(argv[1], "credit") == 0)
        func=compare_int;
    else if (strcmp(argv[1], "name") == 0)
        func=compare_char;
    node=listsort(node,func);
    if (node == NULL) return 1;
    Output(node, fpo);
    fclose(fpi);
    fclose(fpo);
    return 0;
}

せめて、どの行を実行したら動作が止まったかぐらいは特定できるように頑張りましょう。
VC++ならデバッグ機能でブレークポイントが使えますし、そうでなくともprintfなどを随所に織り込むとか、怪しそうな場所にexitの呼び出しを挿入してみるとか、やり方は沢山あります。

とりあえずlistsort関数だけ
以下のようにsave1、save2、pをまず0で初期化していますが、

コード:

SRec *save1=0,*save2=0,*p=0;

ポインタを0で初期化しないでください。
なぜならNULLポインタが0とは限らないからです。
ポインタをNULLで初期化する場合は必ず「NULL」にしてくださいね

コード:

SRec *save1=NULL;

で、save1、save2の更新するところがif文の中にあるので、
while(p->next!=NULL)のループを抜けた後は、
save1、save2はNULLになるケースがあります（かずまさんの指摘よんでますか？）。
なのでここにもif文は必要ですよ。
また、常にtop.nextに入れてるのもおかしいです。これだと常に上書きしてますよね。
リストの末尾に追加したいんでしょう？
そうなるように組んでください。

コード:

        p=&head;
        save1 = p;              // 現在の最大値の一つ前
        save2 = p->next;        // 先頭要素を現在の最大値とする
        while(p->next!=NULL)
        {
            if(func(p->next, save2) > 0)  // 現在の最大値と比較    
            {
                save1=p;        // 削除するために最大のものの１つ手前をsave1として覚える
                save2=p->next;  // 最大のものをsave2
            }
            p=p->next;
        }

まだ、修正したいところはたくさんありますが、とりあえず動くはず。

へにっくすさんが書きました:で、save1、save2の更新するところがif文の中にあるので、
while(p->next!=NULL)のループを抜けた後は、
save1、save2はNULLになるケースがあります（かずまさんの指摘よんでますか？）。
なのでここにもif文は必要ですよ。
また、常にtop.nextに入れてるのもおかしいです。これだと常に上書きしてますよね。
リストの末尾に追加したいんでしょう？
そうなるように組んでください。

どういうif文かよくわからないのですが・・・
全部書き込みは目を通してます。それを書いてる部分がどれというか、そういうことをさしてるということに気づいてませんm(__)m一応直せるところは直してるつもりなので・・・

リストの前にどんどん追加していきたいのですがそれでも間違ってますか？

かずまさんが書きました:

コード:

        p=&head;
        save1 = p;              // 現在の最大値の一つ前
        save2 = p->next;        // 先頭要素を現在の最大値とする
        while(p->next!=NULL)
        {
            if(func(p->next, save2) > 0)  // 現在の最大値と比較    
            {
                save1=p;        // 削除するために最大のものの１つ手前をsave1として覚える
                save2=p->next;  // 最大のものをsave2
            }
            p=p->next;
        }

なぜsave2にするのかがわかりません；
どっちも同じものを比較することになりますよね？？
なんで自分のはだめなのでしょう・・・

れおさんが書きました: なぜsave2にするのかがわかりません；
どっちも同じものを比較することになりますよね？？
なんで自分のはだめなのでしょう・・・

配列で考えてみましょう。

コード:

	int a[4] = { 2, 4, 1, 3 };
	int i, save1, save2;
	i = 0;
	save1 = 0;
	save2 = a[i];
	while (i < 4) {
		if (a[i] > save2) {
			save1 = i;
			save2 = a[i];
		}
        i++;
	}
	// ここでsave2 には最大値が入っている。

確かに、最初に a[0] と a[0] を比較しているのは無駄です。
save2 = a[0];
i = 1;
で始めたほうがいいかもしれませんね。
すなわち、
save1 = &head;
save2 = head.next;
p = save2->next;

れおさんのは、

コード:

	int a[4] = { 2, 4, 1, 3 };
	int i, save1, save2;
	i = 0;
	while (i < 4) {
		if (a[i] > a[i+1]) {  // a[i+1] は範囲外
			save1 = i;
			save2 = a[i];
		}
        i++;
	}
	// ここでsave2 には最大値が入っているとは限らない。

if (func(p->next, p->next->next)) で p->next は NULL ではないけど、
p->next->next は NULL かもしれないということ。
それと、隣同士を比べて最大値が求まりますか？
> 0 をつけなくて正しい比較といえますか？

へにっくすさんが書きました: ポインタを0で初期化しないでください。
なぜならNULLポインタが0とは限らないからです。
ポインタをNULLで初期化する場合は必ず「NULL」にしてくださいね

ポインタを 0 で初期化することは規格違反ではありません。
規格書によると次のようにも書けます。

コード:

SRec *save1 = 6 - 2 * 3;  // C と C++ 
Srec *save2 = (void *) 0; // C

null pointer の内部表現(ビットパターン)が全ビット 0 という保証はないので
memset(&save1, 0, sizeof(save1)) はダメな場合がありますが、
save1 = 0; は問題ありません。

C の規格書
- ISO/IEC 9899:1999 または ISO/IEC 9899:201x のドラフト
6.3.2.3 Pointers
An integer constant expression with the value 0, or such an expression
cast to type void *, is called a null pointer constant.

- JIS X 3010:2003 (ISO/IEC 9899:1999 の翻訳)
6.3.2.3 ポインタ
値0をもつ整数定数式又はその定数式を型void * にキャストした式を,
空ポインタ定数(null ponter constant)と呼ぶ。

C++ の規格書
- ISO/IEC 14882-1998
4.10 Pointer conversions [conv.ptr]
A null pointer constant is an integral constant expression (5.19) rvalue
of integer type that evaluates to zero.

- N3242 (ISO/IEC 14882-2011 の最終ドラフト)
4.10 Pointer conversions [conv.ptr]
A null pointer constant is an integral constant expression (5.19) prvalue
of integer type that evaluates to zero or a prvalue of type std::nullptr_t.

* prvalue というのは "pure" rvalue です。

かずまさんが書きました:null pointer の内部表現(ビットパターン)が全ビット 0 という保証はないので
memset(&save1, 0, sizeof(save1)) はダメな場合がありますが、
save1 = 0; は問題ありません。

そうですか。指摘ありがとうございます。
でも個人的には NULL とすべきなのかなと思いますがね。
(キャストもせずに単純に0を入れるよりはだいぶましかと ^^;)

たしかに隣同士しかくらべてなかったです・・・
全然気づいてなかった＞＜
今確認したのはそこだけなので
また後でくわしく考えてみます。

なるほどなんとなくそれでいけるというのはわかりました。
ただ、自分でそれを思いつくかどうか・・・＞＜

他にもどこかダメなところがあるようですが、どこなのでしょうか？？
一応示してくれたものを訂正して、思った通りの実行結果になりますが・・・

れおさんが書きました:他にもどこかダメなところがあるようですが、どこなのでしょうか？？

(1) listsort の第1引数の node が使われていないこと
listsort はグローバル変数の head をソートしています。
これは、Input が入力されたリストを返しながら、
head にも結果を残していることからうまくいっています。
グローバル変数をこんな風に使っていると、2組のデータを 2つのリストに
別々に読み込み、別々にソートすることがむずかしくなります。

(2) Input が毎回リストの末尾を探しながら入力データを追加していること
あとでソートするんだから、先頭に挿入していくだけでよいはずです。
どうしても末尾に追加したいんだったら、一つ前に追加したデータの場所を
tail に覚えておいて、次のデータをそこにつなぐだけで済みます。

(3) malloc したデータ領域を解放していないこと
今回は、一仕事終わったらすぐにプログラムが終了するので問題ありませんが、
プログラムを終了させずに次の仕事をやり始めるなら、
元のデータはメモリーリークとなります。

(4) compare_char で strcmp を 2回呼び出していること
比較関数は、小さい、等しい、大きいの 3種類が返せればよいので、負、ゼロ、正
の値を返せばよく、return strcmp(str1->name, tm2->name); でよい。

(5) 名前が悪い
compare_char は char を比較しているというよりも「文字列」を比較しています。
というか、SRec の中の name を比較しているんだから compare_name のほうがよい。
他もそうです。compare_gpa、compare_credit のほうがよい。
例えば、学生番号が int id; というメンバが SRec に追加された場合、compare_int だと
credit の比較と区別がつきません。
compare_credit だと、credit がそんなに大きな値にならないということで、差をとっても
オーバーフローが起こらないので、return tmp1->credit - tmp2->credit; と書けます。
func というのもよくない。比較関数だと分かるように compare とか comp にしたほうがよい。
Input、Output が大文字で始まるのに、listsort は小文字。

（３）（４）（５）だけなおしてみましたが　実行結果がうまくいきません。

コード:

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
typedef struct school_record SRec;
struct school_record {
    float gpa;
    int credit;
    char name[200];
    SRec *next;
};
SRec head;                					 // head.next がリストの先頭要素を指す

SRec* Listsort(SRec *node,int (*comp)(const void *,const void *))
{
    SRec top;        						//top.nextがソート後のリストの先頭要素へのポインタ
    SRec *save1=NULL,*save2=NULL,*p=NULL;
    top.next=NULL;
    while(head.next!=NULL)
    {
    	p=&head;
    	save1 = p;              // 現在の最大値の一つ前
    	save2 = p->next;        // 先頭要素を現在の最大値とする
    	while(p->next!=NULL)
    	{
    		if(comp(p->next, save2) > 0)  // 現在の最大値と比較
    			{
    			save1=p;        // 削除するために最大のものの１つ手前をsave1として覚える
    			save2=p->next;  // 最大のものをsave2
    			}
    		p=p->next;
    	}
        save1->next=save1->next->next;      //リストの削除
        save2->next=top.next;
        top.next=save2;
    }
    free(node);
    return top.next;
}

int compare_gpa(const void * a, const void * b)
{
    SRec* tmp1 = (SRec*) a;
    SRec* tmp2 = (SRec*) b;
    return tmp1->gpa - tmp2->gpa;
}
int compare_credit(const void* a, const void* b)
{
    SRec* tmp1 = (SRec*) a;
    SRec* tmp2 = (SRec*) b;
    return tmp1->credit - tmp2->credit;
}

int compare_name(const void * a, const void * b)
{
    SRec* tmp1 = (SRec*) a;
    SRec* tmp2 = (SRec*) b;
    return strcmp(tmp1->name, tmp2->name);
}

SRec* Input(FILE* fpi)
{
    SRec *node=NULL;
    SRec *tail=NULL;
    head.next = NULL;/*これが NULL だとリストにはまだ 1つもノードがないことを表す。
                       これが NULL でないとリストに最低 1つはノードがあることになる。
                       実際にはまだリストは空っぽだから NULL する。*/
    tail = &head;
    while (1)
    {
        node = (SRec *) malloc(sizeof(SRec));//これは配列ではない
        if (node == NULL)
        {
            fprintf(stderr,"メモリ不足");
            return NULL;
        }
        if (fscanf(fpi, "%f%d%s", &node->gpa, &node->credit, node->name) != 3)
        {
            break;  // 読み込みに失敗したら EOF (feof は使わないほうがよい)
        }
        node->next = NULL;  // この node を末尾要素とする
        for (tail = &head; tail->next != NULL; tail = tail->next) ;// 現在のリストの末尾要素を探す
        tail->next = node;  // 見つかった末尾要素に node を追加する。
    }
    free(node);
    return head.next;  // リストの先頭要素へのポインタを返す。
}
void Output(SRec *node, FILE* fpo)
{
    for (; node != NULL; node = node->next)
    {
        fprintf(fpo,"%3.1f %d %s\n", node->gpa, node->credit, node->name);
    }
    free(node);
}
int main(int argc, char *argv[])
{
    SRec *node=NULL;
    FILE *fpi, *fpo;
    int (*comp)(const void*,const void*);
    if (argc != 4)
    {
        fprintf(stderr, "Illegal number of argument.\n");
        return (-1);
    }
    if ((fpi = fopen(argv[2], "r")) == NULL)
    {
        fprintf(stderr, "Can't open input file <%s>.\n", argv[2]);
        return (-1);
    }
    if ((fpo = fopen(argv[3], "w")) == NULL)
    {
        fprintf(stderr, "Can't open output file <%s>.\n", argv[3]);
        return (-1);
    }
    node = Input(fpi);
    if (node == NULL) return 1;
    if (strcmp(argv[1], "gpa") == 0)
        comp=compare_gpa;//&はどちらでもいい
    else if (strcmp(argv[1], "credit") == 0)
        comp=compare_credit;
    else if (strcmp(argv[1], "name") == 0)
        comp=compare_name;
    node=Listsort(node,comp);
    if (node == NULL) return 1;
    Output(node, fpo);
    fclose(fpi);
    fclose(fpo);
    return 0;
}

入力ファイル
3.5 27 jiro
2.7 24 taro
3.7 30 saburo
12.1 40 kiriyama
100 100 nakagawa
0.13 4 numai

gpaでの並べ替え（出力ファイル）
0.1 4 numai
2.7 24 taro
3.7 30 saburo
0.0 6625256 jiro
12.1 40 kiriyama
100.0 100 nakagawa

creditでの並べ替え（出力ファイル）
0.1 4 numai
2.7 24 taro
0.0 4069352 jiro
3.7 30 saburo
12.1 40 kiriyama
100.0 100 nakagawa

ってなりました(+o+)
ファイルの中身は間違ってない（大文字小文字など）とおもうのですが・・・＞＜

れおさんが書きました:（３）（４）（５）だけなおしてみましたが　実行結果がうまくいきません。

compare_gpa ですが、credit は小さな int だから差でいいけれど、
gpa は double だから、差ではいけません。
3.7 と 3.5 の差は 0.2。これが int に変換されて 0 が返ってきます。
元に戻してください。

Listsort の中では余計な malloc を実行していないのに、free(node); があって
リストをぶっ壊しています。

あぁ・・・
そういえばそういうのありましたね・・・OTZ
ダメダメですね・・・・

node使ってやってみます(+o+)」

かずまさんが書きました:Listsort の中では余計な malloc を実行していないのに、free(node); があって
リストをぶっ壊しています。

ここで free しているのは (3) への対応のつもりだったんでしょうか？
Input の中の malloc で確保した領域は、Output で表示が済むまで free してはいけません。
Output のあとで、リストをたどって、malloc の個数分の free が必要です。

一応そういうつもりでした。教えてもらったのは、mallocがでてきたらfreeするということだったので、コート内に mallocという単語がでてきた一番最後でfreeすると思ってました。
で今回はmallocであるnodeをどこの他の関数にもわたしているのでその関数の最期でいれました。
nodeをわたしているからまだ渡していないところでfreeするなということですよね？
だから今回freeをいれるのは、Inputと、Output（もしくはmainの最後）でいいんですよね？

下はまだnodeはつかってませんが（２）から（５）に対応できたはずのものです。あとは（１）だけのはず・・・＞＜

コード:

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
typedef struct school_record SRec;
struct school_record {
    float gpa;
    int credit;
    char name[200];
    SRec *next;
};
SRec head;                					 // head.next がリストの先頭要素を指す

SRec* Listsort(SRec *node,int (*comp)(const void *,const void *))
{
    SRec top;        						//top.nextがソート後のリストの先頭要素へのポインタ
    SRec *save1=NULL,*save2=NULL,*p=NULL;
    top.next=NULL;
    while(head.next!=NULL)
    {
    	p=&head;
    	save1 = p;              // 現在の最大値の一つ前
    	save2 = p->next;        // 先頭要素を現在の最大値とする
    	while(p->next!=NULL)
    	{
    		if(comp(p->next, save2) > 0)  // 現在の最大値と比較
    			{
    			save1=p;        // 削除するために最大のものの１つ手前をsave1として覚える
    			save2=p->next;  // 最大のものをsave2
    			}
    		p=p->next;
    	}
        save1->next=save1->next->next;      //リストの削除
        save2->next=top.next;
        top.next=save2;
    }
    return top.next;
}

int compare_gpa(const void * a, const void * b)
{
    SRec* tmp1 = (SRec*) a;
    SRec* tmp2 = (SRec*) b;
    if (tmp1->gpa == tmp2->gpa)
        return 0;
    else if (tmp1->gpa > tmp2->gpa)
        return 1;
    else
        return -1;                         //これは - ではできない∵int
}
int compare_credit(const void* a, const void* b)
{
    SRec* tmp1 = (SRec*) a;
    SRec* tmp2 = (SRec*) b;
    return tmp1->credit - tmp2->credit;
}

int compare_name(const void * a, const void * b)
{
    SRec* tmp1 = (SRec*) a;
    SRec* tmp2 = (SRec*) b;
    return strcmp(tmp1->name, tmp2->name);
}

SRec* Input(FILE* fpi)
{
    SRec *node=NULL;
    SRec *tail=NULL;
    head.next = NULL;/*これが NULL だとリストにはまだ 1つもノードがないことを表す。
                       これが NULL でないとリストに最低 1つはノードがあることになる。
                       実際にはまだリストは空っぽだから NULL する。*/
    tail = &head;
    while (1)
    {
        node = (SRec *) malloc(sizeof(SRec));//これは配列ではない
        if (node == NULL)
        {
            fprintf(stderr,"メモリ不足");
            return NULL;
        }
        if (fscanf(fpi, "%f%d%s", &node->gpa, &node->credit, node->name) != 3)
        {
            break;  // 読み込みに失敗したら EOF (feof は使わないほうがよい)
        }
        node->next = NULL;  // この node を末尾要素とする
        /*for (tail = &head; tail->next != NULL; tail = tail->next) ;現在のリストの末尾要素を探す*/
        tail->next = node;  // 見つかった末尾要素に node を追加する。
        tail= node;

    }
    free(node);
    return head.next;  // リストの先頭要素へのポインタを返す。
}
void Output(SRec *node, FILE* fpo)
{
    for (; node != NULL; node = node->next)
    {
        fprintf(fpo,"%3.1f %d %s\n", node->gpa, node->credit, node->name);
    }
    free(node);
}
int main(int argc, char *argv[])
{
    SRec *node=NULL;
    FILE *fpi, *fpo;
    int (*comp)(const void*,const void*);
    if (argc != 4)
    {
        fprintf(stderr, "Illegal number of argument.\n");
        return (-1);
    }
    if ((fpi = fopen(argv[2], "r")) == NULL)
    {
        fprintf(stderr, "Can't open input file <%s>.\n", argv[2]);
        return (-1);
    }
    if ((fpo = fopen(argv[3], "w")) == NULL)
    {
        fprintf(stderr, "Can't open output file <%s>.\n", argv[3]);
        return (-1);
    }
    node = Input(fpi);
    if (node == NULL) return 1;
    if (strcmp(argv[1], "gpa") == 0)
        comp=compare_gpa;//&はどちらでもいい
    else if (strcmp(argv[1], "credit") == 0)
        comp=compare_credit;
    else if (strcmp(argv[1], "name") == 0)
        comp=compare_name;
    node=Listsort(node,comp);
    if (node == NULL) return 1;
    Output(node, fpo);
    fclose(fpi);
    fclose(fpo);
    return 0;
}

れおさんが書きました:

コード:

void Output(SRec *node, FILE* fpo)
{
    for (; node != NULL; node = node->next)
    {
        fprintf(fpo,"%3.1f %d %s\n", node->gpa, node->credit, node->name);
    }
    free(node);
}

指摘されているのは、たぶんこういうことではないかと思います。

こういう場所にfree()を書くのではなくて（しかも、連結リストの最後の次（つまり存在しないノード）「しか」開放していない、
という点で使い方が間違っている）、開放するための関数か何かを別個に用意して、
連結リストのノードを先頭から順番にたどっていきながらfree()するべきではないか。

Output()は、連結リストの内容出力に特化すべきで、出力以外のよけいなことはしない方がよい。

あ～なるほどです。
具体的にはわかりませんが、ループの最後でfreeしたらいけるような気がするのですが・・・
関数をつくらないとむりですかね？

グローバル変数を使わないようにしてみました。
head は listL、top は listS、save2 は nodeM と名を変えています。

コード:

#include <stdio.h>
#include <stdlib.h>
#include <string.h>

typedef struct school_record SRec;
struct school_record {
    float gpa;
    int credit;
    char name[200];
    SRec *next;
};

SRec* Sort(SRec *srec, int (*comp)(const void *, const void *))
{
    SRec listL, listS;  // ソート前とソート後のリスト

    listL.next = srec;  // listL.next はソート前のリストへのポインタ
    listS.next = NULL;  // listS.next はソート後のリストへのポインタ

    while (listL.next != NULL) {  // listL が空になるまで繰り返す
        SRec *prevM = &listL;        // 現在の最大値の一つ前を指す
        SRec *nodeM = listL.next;    // 先頭要素を現在の最大値とする
        SRec *p;                     // listL を順に見ていくポインタ
        for (p = &listL; p->next != NULL; p = p->next) {
            if (comp(p->next, nodeM) > 0) {  // 現在の最大値と比較
                prevM = p;           // 新しい最大値の一つ前を指す
                nodeM = p->next;     // 新しい最大値を指す
            }
        }
        prevM->next = nodeM->next;   // nodeM を listL から削除
        nodeM->next = listS.next;    // nodeM を listS の先頭に挿入
        listS.next = nodeM;          // nodeM を listS の先頭に挿入
    }
    return listS.next;  // ソート後のリストへのポインタを返す
}

int compare_gpa(const void * a, const void * b)
{
    SRec *tmp1 = (SRec *) a;
    SRec *tmp2 = (SRec *) b;
    return (tmp1->gpa < tmp2->gpa) ? -1 : (tmp1->gpa > tmp2->gpa);
}

int compare_credit(const void* a, const void* b)
{
    SRec *tmp1 = (SRec *) a;
    SRec *tmp2 = (SRec *) b;
    return tmp1->credit - tmp2->credit;
}

int compare_name(const void * a, const void * b)
{
    SRec *tmp1 = (SRec *) a;
    SRec *tmp2 = (SRec *) b;
    return strcmp(tmp1->name, tmp2->name);
}

SRec *Input(FILE *fpi)
{
    SRec *node;         // データを読み込むノード
    SRec listL;         // 入力データのリスト
    listL.next = NULL;  // listL はまだデータを持っていない
    while (1) {
        node = (SRec *) malloc(sizeof(SRec));
        if (node == NULL) {
            fprintf(stderr,"メモリ不足");
            return NULL;
        }
        if (fscanf(fpi, "%f%d%s", &node->gpa, &node->credit, node->name) != 3)
            break;      // 読み込みに失敗したら EOF
        node->next = listL.next; // listL の先頭に読み込んだ node を挿入
        listL.next = node;       // listL の先頭に読み込んだ node を挿入
    }
    free(node);         // データを読み込まなかったノードを解放
    return listL.next;  // リストの先頭要素へのポインタを返す
}

void Output(SRec *srec, FILE* fpo)
{
    SRec *node;
    for (node = srec; node != NULL; node = node->next)
        fprintf(fpo, "%6.1f %4d  %s\n", node->gpa, node->credit, node->name);
}

void Free(SRec *srec)
{
    SRec *node, *next;
    for (node = srec; node != NULL; node = next) {
        next = node->next;
        free(node);
    }
}

int main(int argc, char *argv[])
{
    FILE *fpi, *fpo;
    int (*comp)(const void *, const void *);  // 比較関数へのポインタ
    SRec *srec;                     // 成績データのリストへのポインタ

    if (argc != 4) {
        fprintf(stderr, "Illegal number of argument.\n");
        return (-1);
    }
    if ((fpi = fopen(argv[2], "r")) == NULL) {
        fprintf(stderr, "Can't open input file <%s>.\n", argv[2]);
        return (-1);
    }
    if ((fpo = fopen(argv[3], "w")) == NULL) {
        fprintf(stderr, "Can't open output file <%s>.\n", argv[3]);
        return (-1);
    }
    if (strcmp(argv[1], "gpa") == 0)
        comp = compare_gpa;
    else if (strcmp(argv[1], "credit") == 0)
        comp = compare_credit;
    else if (strcmp(argv[1], "name") == 0)
        comp = compare_name;
    else {
        fprintf(stderr, "key must be gpa, credit, or name\n");
        return 1;
    }
    srec = Input(fpi);
    if (srec == NULL) return 1;
    srec = Sort(srec, comp);
    Output(srec, fpo);
    Free(srec);
    fclose(fpi);
    fclose(fpo);
    return 0;
}

char name[200]; が無駄にメモリを食っているのが不満ですが。

グローバル変数は使わないほうがいいんですかね？
コンパイルの速度など考えると同じくらいなんですかね？
多分次ファイルの中身を何万個とかにしてやるのでできるだけ速いほうが好ましいので・・・

char[200]は仕様というかきめられてるのでおいといてくださいｗ

Input関数では最後に１回しかfreeしてないのに、Free関数ではループで回してfreeしているのはなぜなんですか？
どういう違いがあるのかわかりません・・・
ネットで調べてたらたしかにループ内でやってるやつもあるのですが、どういうときそうするのかわからなくて・・・

れおさんが書きました:グローバル変数は使わないほうがいいんですかね？
コンパイルの速度など考えると同じくらいなんですかね？
多分次ファイルの中身を何万個とかにしてやるのでできるだけ速いほうが好ましいので・・・

速度の観点から言えば、グローバル変数にしたところであまり変わらない気がします。
ただ、やたらめったらグローバル変数を使うと、そのうち整理ができなくなり破たんしますね。
あくまでも必要最低限にとどめるべきだと思います。

れおさんが書きました:Input関数では最後に１回しかfreeしてないのに、Free関数ではループで回してfreeしているのはなぜなんですか？

コメントに書いてあるがな。
と一言で済ますのはあまり親切でない気がするので、、(^^;

Input関数では、

1)メモリ確保したものをnodeに入れる。
2)fscanfでファイルからnodeのgpa、credit、nameを読み込む。
3)fscanfの読込が成功したら、node->nextにlistL.nextを入れ、listL.nextにnodeを登録する。
4)1)に戻る。

ここまでは分かるよね。
では、もしfscanfの読込が失敗したら、どうなるか？
3)の後のbreak;文でwhileループを抜けるよね。nodeを登録することなく(読み込めなかったんだから、登録する必要もないよね)。
登録しないんだから、nodeを解放する必要があります。
これが最後に１回、解放する理由です。

Free関数ではプログラムの最後に呼ばれるものだから、全部解放する必要があります。

この説明で分かるかな…

前半はわかりました＾＾

『Free関数ではプログラムの最後に呼ばれるものだから、全部解放する必要があります。』
だけがちょっと。ただへにっくすさんのを言い換えただけになってるのかもしれませんが・・m(__)m

かずまさんのコードでいうと、
Inputでは最後nodeをよびだしてるのに、中になにもいれてないから最後のループがおわったときだけfreeしてるんですよね？
Freeではsrecを最初からNULLになるまでfreeするのは、最後だからもうどのデータもいらないから全部最初からfreeするということですか？

れおさんが書きました:かずまさんのコードでいうと、
Inputでは最後nodeをよびだしてるのに、中になにもいれてないから最後のループがおわったときだけfreeしてるんですよね？

何か言い回しが気に入らないです…(=_=;
ループの中で必ず最初にnodeへ確保したメモリのポインタを代入している。
そのポインタをリストに入れていない以上、必要ないポインタは解放するべき。ということ。

れおさんが書きました:Freeではsrecを最初からNULLになるまでfreeするのは、最後だからもうどのデータもいらないから全部最初からfreeするということですか？

その認識でよい。
メモリ確保したものは、必ず自己責任で解放する必要があるのです。
C/C++言語を使う以上は、そのことを忘れないでください。

わかりましたー
ありがとうございましたm(_ _)m

また新しいスレたてますのでこれはここで終了させてください

みなさんありがとうございましたm(_ _)m次回もお願いしますm(_ _)m

プログラマ専用SNS ミクプラ

成績データのソート（連結リスト・その２）

成績データのソート（連結リスト・その２）

Re: 成績データのソート（連結リスト・その２）

Re: 成績データのソート（連結リスト・その２）

Re: 成績データのソート（連結リスト・その２）

Re: 成績データのソート（連結リスト・その２）

Re: 成績データのソート（連結リスト・その２）

Re: 成績データのソート（連結リスト・その２）

Re: 成績データのソート（連結リスト・その２）

Re: 成績データのソート（連結リスト・その２）

Re: 成績データのソート（連結リスト・その２）

Re: 成績データのソート（連結リスト・その２）

Re: 成績データのソート（連結リスト・その２）

Re: 成績データのソート（連結リスト・その２）

Re: 成績データのソート（連結リスト・その２）

Re: 成績データのソート（連結リスト・その２）

Re: 成績データのソート（連結リスト・その２）

Re: 成績データのソート（連結リスト・その２）

Re: 成績データのソート（連結リスト・その２）

Re: 成績データのソート（連結リスト・その２）

Re: 成績データのソート（連結リスト・その２）

Re: 成績データのソート（連結リスト・その２）

Re: 成績データのソート（連結リスト・その２）

Re: 成績データのソート（連結リスト・その２）

Re: 成績データのソート（連結リスト・その２）

Re: 成績データのソート（連結リスト・その２）

Re: 成績データのソート（連結リスト・その２）

Re: 成績データのソート（連結リスト・その２）

Re: 成績データのソート（連結リスト・その２）

Re: 成績データのソート（連結リスト・その２）

Re: 成績データのソート（連結リスト・その２）