switchとメモリ取得位置

質問

#include <stdlib.h>
#include <string.h>
#include <ctype.h>
#include <string.h>
#include <stdio.h>
#include <stdlib.h>
#define TRUE 1
#define FALSE -1
char* get(char **p_str);
char *get_line(char buf[]);
int comp_rtn(const void *p1, const void *p2);

typedef struct {
int number;
char class_type[10];
char name[15];
char subject[10];
int ten;

} my;
my *data;
void myswap(my *p, my *q);
int main(int argc, char* argv[])
{
FILE *fp;
int field = 0, line = 0;
char buf[1000], *str;
char *bufFormat;
char *bufG;

bufG = (char *)malloc(1000);
if(bufG == NULL){
printf("メモリ不足");
free(bufG);
}

int line2 = 0;

if((fp=fopen("jjj.txt","r"))==NULL){
printf("ファイルが開けません");
}

while(fgets(buf, 1000, fp) != NULL){
line2++;
}
fclose(fp);

if((fp=fopen("jjj.txt","r"))==NULL){
　　printf("ファイルが開けません");
}

data = (my *)malloc(sizeof(my) * line2);
if(data == NULL){
printf("メモリ不足");
free(data);
}

while(fgets(buf,1000,fp) != NULL){
bufFormat =(char *)malloc(strlen(buf) + 1);
if(bufFormat == NULL){
printf("メモリ不足");
free(data);
}
bufFormat = get_line(buf);
str = bufFormat;
while(*str != '\0'){
bufG = get(&str);
switch(field){
　　　　　case 0:
data[line].number = atoi(bufG); break
case 1:
strcpy(data[line].class_type, bufG);
break;
case 2:
strcpy(data[line].name, bufG);
break;
case 3:
strcpy(data[line].subject, bufG);
break;
case 4:
data[line].ten = atoi(bufG);
break;
}
str++;
field++;
}
line++;
field = 0;
}

fclose(fp);
qsort(data,line,sizeof(my),comp_rtn);

for(int m = 0; m < line; m++){
printf("%d\n", data[m].number);
printf("%s\n", data[m].class_type);
printf("%s\n", data[m].name);
printf("%s\n", data[m].subject);
printf("%d\n", data[m].ten);
printf("\n");
}
free(data);

return 0;

}

void myswap(my *p, my *q)
{
my temp;
temp = *p;
*p = *q;
*q = temp;
}
char *get(char **p_str)
{
int i;
char *str;
str = *p_str;

static char bufG[1000];
for(i = 0; *str != ',' && *str != '\0' ; i++){
if(*str == '\n'){
bufG[i] = '\0';
}
else if(*str == '\\'){
str++;
if(*str == 'c'){
bufG[i] = ',';
}
else if(*str == '"'){
bufG[i] = '"';
}
}
else{
bufG[i] = *str;
}

str++;
}

bufG[i] = '\0';
*p_str = str;
return bufG;
}

char *get_line(char buf[])
{
int in_quotation = FALSE, i = 0;
char* str = buf;

static char bufG[1000];
while(*str != '\0'){
if(*str=='"'){
if(in_quotation == TRUE){
str++;
if(*str == '"'){
bufG[i] = '\\';
i++;
bufG[i] = '"';
i++;
}
else{
in_quotation = FALSE;
bufG[i] = *str;
i++;
}
}
else{
in_quotation = TRUE;
}
}
else{
switch(*str){
case '\n':
if(in_quotation == TRUE){
bufG[i] = '\\';
i++;
bufG[i] = 'n';
i++;
}
else{
bufG[i] = *str;
i++;
}
break;
case ',':
if(in_quotation == TRUE){
bufG[i] = '\\';
i++;
bufG[i] = 'c';
i++;
}
else{
bufG[i] = *str;
i++;
}
break;
default:
bufG[i] = *str;
i++;
}
}
str++;
}
bufG[i] = '\0';
return bufG;
}

構造体メンバをポインタで宣言する方法はもうできていて
配列の方も組んでいるのですが
比較関数など所々省いてますがこのプログラムに対し以下のことをいわれました

1,switch文のfieldの値を数字じゃなく分かりやすいのに変えよ

2,mainのすぐしたの bufG = (char * )malloc(1000)が
なかなか使用されないのにここでメモリを取るのはおかしい
3,これを使うまでの間にエラーが発生したときのfreeがない

と言われました。

1ですが確かCではswitch文のcase式は整数型定数でなければならない
とあるので無理な気もするのですが、方法ありますか？

2に関してはよくわかりません。効率がよくないのでしょうか？
どの場所がいいのでしょうか

3に関してはどういうことなのかもわかりません。

この３点について教えて下さい。

回答No.5

chie65536（@chie65535） · ベストアンサー

1.
enum {
number_col,
class_type_col,
name_col,
subject_col,
ten_col
};
(略)
case number_col:
data[line].number = atoi(bufG); break
case class_type_col:
strcpy(data[line].class_type, bufG);
break;
case name_col:
strcpy(data[line].name, bufG);
break;
case subject_col:
strcpy(data[line].subject, bufG);
break;
case ten_col:
data[line].ten = atoi(bufG);

2.
get関数、get_line関数で
static char bufG[1000];
と定義して、静的なメモリのアドレスを
return bufG;
で返している。

で、mainは
bufG = get(&str);
と、静的なメモリのアドレスをbufGに受け取っている。

つまり「mallocしたメモリのアドレスをbufGに入れてあるのをすっかり忘れて、bufGを変更しちゃってるので、mallocしたメモリを２度とfree出来なくなっている」と言う状態になっている。

「効率が良くない」とかの問題じゃなく
・mallocしたメモリがfreeできず、メモリがリークしている
・そもそも、静的なメモリを使用しているのだから、mallocそのものが不要

同様の事が、bufFormatでも起きている。

bufFormat =(char *)malloc(strlen(buf) + 1);
(中略)
bufFormat = get_line(buf);

ここでも「mallocしたメモリのアドレスをbufFormatに入れてあるのをすっかり忘れて、bufFormatを変更しちゃってるので、mallocしたメモリを２度とfree出来なくなっている」と言う状態になっている。

3.
「mallocしたすべてのメモリは、freeするまで、保持しつづけなければならない」「mallocしたすべてのメモリは、プログラムが終了する前までに、プログラマが責任を持ってすべてfreeしなければならない」と言う事が理解できていません。

しかも、質問者さんは「エラー判定がまったく出来ていない」です。

質問者さんがやってるのは「エラー判定のフリ」だけで、何もしてないのと同じです。

エラー判定とは「エラーのため、必要な物を確保、設定出来なかった時に＜＜＜＜それ以上、処理を継続しないため＞＞＞＞の処理」です。

「＜＜＜＜」と「＞＞＞＞」で囲った部分を１００回読んで下さい。

１００回読んでから
if(bufG == NULL){
printf("メモリ不足");
free(bufG);
}
とか
if((fp=fopen("jjj.txt","r"))==NULL){
printf("ファイルが開けません");
}
とか
if(data == NULL){
printf("メモリ不足");
free(data);
}
とかの部分を良く見てください。

これらのエラー処理は「エラーだ、と報告しているだけで、平気で処理を継続してしまっていて、必要な変数が設定されてないまま、必要なファイルが開けてないまま、プログラムを中断してない」です。

通常、エラー処理というのは、以下のようにします。
p1 = malloc(～～～);
if (p1 == NULL) {
printf("メモリ不足\n");
return 0;//これ以上処理できないので呼び出し元に帰る
}
p2 = malloc(～～～);
if (p2 == NULL) {
free(p1);//p1はmalloc出来てるので、freeして後始末する
printf("メモリ不足\n");
return 0;//これ以上処理できないので呼び出し元に帰る
}
p3 = malloc(～～～);
if (p3 == NULL) {
free(p1);//p1はmalloc出来てるので、freeして後始末する
free(p2);//p2はmalloc出来てるので、freeして後始末する
printf("メモリ不足\n");
return 0;//これ以上処理できないので呼び出し元に帰る
}
fp=fopen(～～～);
if (fp == NULL) {
free(p1);//p1はmalloc出来てるので、freeして後始末する
free(p2);//p2はmalloc出来てるので、freeして後始末する
free(p3);//p3はmalloc出来てるので、freeして後始末する
printf("ファイルが開けません\n");
return 0;//これ以上処理できないので呼び出し元に帰る
}
p4 = malloc(～～～);
if (p4 == NULL) {
free(p1);//p1はmalloc出来てるので、freeして後始末する
free(p2);//p2はmalloc出来てるので、freeして後始末する
free(p3);//p3はmalloc出来てるので、freeして後始末する
fclose(fp);//fpはfopen出来てるので、fcloseして後始末する
printf("メモリ不足\n");
return 0;//これ以上処理できないので呼び出し元に帰る
}
//全てのメモリ、ファイルが準備出来たので、ここで処理開始
(中略)
//すべての処理が終ったら、すべてfreeし、すべてfcloseする
free(p4);
fclose(fp);
free(p3);
free(p2);
free(p1);
//すべてfreeし、すべてfcloseしているのでreturnして、プログラムを終了しても良い
return 0;

質問者さんのプログラムは、こういう「後始末」がまったく出来てないので「どうにかしろ」と言われたのです。

以下蛇足。

「表」の漢字でバグが起きるのが直ってませんね。

http://okwave.jp/qa5122320.html
'\\'を'_'に変えたとしても「表」でバグる代わりに「廟」でバグるようになるだけで、何の問題解決にもなってません。

ちゃんと「漢字は漢字として処理して、漢字の第２バイトの'\\'は、'\\'として特殊処理しないでスキップする」と言う処理をしましょう。

もし「漢字の判定が出来ない」と言うなら、漢字の第２バイトに来ない文字だけを特殊文字に使用しましょう。

漢字の第２バイトには
!"#$%&'()*+,-./0123456789:;<=>?
の文字は来ないので、この文字を特別扱いするなら、漢字の判定は不要です。

エラー処理の件もそうですが「対処療法的に目に見える事象やバグにだけ対処し、根本的な原因や理由を理解、解決しようとしない姿勢」は、質問者さんの悪い癖です。

こういう姿勢が身についてしまっているので
＞3に関してはどういうことなのかもわかりません。
と言うように「何が悪いのか、理解できない」と言う状況に陥るのです。

お礼

そんな姿勢になってる感はありますね。
わかりやすい解説ありがとうございました。

わかったこと
・静的なメモリについての理解ができていなかった
・エラー処理に関しては本文ではreturn 0は入ってますが
それ以外は不足していました。
・漢字の２バイト

free(bufFormat)ってすると落とされててあれ・・って
なってたんですが、この説明でわかりました。

皆様貴重な意見ありがとうございました。

回答No.4

麻野 なぎ（@AsanoNagi）

このコードなら、１．については、
列挙体（enum）を使った方が良いと思います。

switch(field) で、分岐して、しかも、
field++; で処理を切り替えています
から、#define による定義だと、

#define isNumber 0
#define isClass 1
#define isName 3
#define isSubject 3
なんてなっていて、正体不明のバグに
引っかかるかもしれません。

enum FieldType {isNumber, isClass, ...}
だと、
enum FiledType field;
で、
filed = isNumber;
とかできますし、
filed++;
で、定義した順序で、抜けなく処理できる
ことがわかりますから。

あと、Ｃ＋＋なら、型チェックも効きます。

回答No.3

php504

1.
例えば
case -1:
よりも
case FALSE:
のほうがわかりやすいという意味でしょう

2.3.
bufG = get(&str);
で別のポインタを代入してるようなので
bufG = (char *)malloc(1000);
if(bufG == NULL){
printf("メモリ不足");
free(bufG);
}
の部分は削除しても問題ないと思いますよ

回答No.2

rinkun

1は#defineでも使って整数の代わりにマクロ名を使う。
2はbufGの使い方を見るとmallocした領域を使っていないように見える。そもそもmallocが必要ないのでは?
3は実際は途中のエラーのあるなしに関わらず2でmallocした領域を正しくfreeしていない。
bufGはfreeする前に他のアドレスで上書きされてしまっている。

なお、メモリ不足でmallocがNULLを返した場合にその変数をfreeすることには意味がない。むしろfreeすべきなのは他の既に獲得した領域を保持する変数だし、また必要な領域が確保できていないのに異常終了させずに先に進んでは不味いだろう。

回答No.1

D-Matsu

1. #defineかconstを使えって事です。
2. 主にデバッグ効率の問題です。大抵の場合は使う直前のところ、このコードならwhileループの直前で確保するように書きます。
3. fopen()などでエラーが発生したときにbufGを解放してないということです。

あと、bufG==NULLの時のfree(bufG)は不要というか無意味です。あっても問題ありませんが、もともと確保できてないからNULLが返っているので。