C言語 ポインタ型引数の呼び出しについて２

質問

C言語　ポインタ型引数の呼び出しについて２

以前質問した関数ですが、ネットを検索していたら同じような引数の関数を見つけました。
多分これを参考にしたのではないかと思います。

char* GetCSV(char** pBuf)
{
char *p = *pBuf,*q = p;
if (!p) return p;
if (*p == '"') {
*p++ = 0;
do ++q; while (*q && *q != '"');
if (*q == '"') *q++ = 0;
}
if ((*pBuf = strpbrk(q,",")) != 0) *(*pBuf)++ = 0;
return p;
}

当方がやりたい処理はVB6でCSVをリードして処理しているのですが、処理速度が遅いため
Ｉ／ＯをしているところをＣに変更して処理速度が向上するか確認したいのです。
他にも色々調べたのですがstrtokを使って処理されている方が多いようなのですが、
"1,,2,3"と言うデータの場合に"1" "2" "3"と返ってくるようで、
私のほしい値としては"1" "" "2"　"3"というように返ってきてほしいのです。

上記のGetCSVでは"1"　""　"2"　"3"に返ってくるようです。

何とか動くようにと下の関数をかぶして実行して見たところ結果は正しかったのですが
どうもメモリリークをしているように思います（タスクマネージャーのメモリ使用量は増えていく）

int split(char *ary[], char *s)
{
　　int len = 0;
　　int lsize = 0;
char *strBuff[BUF_SIZE];

*strBuff = strdup(s);
lsize = strlen(*strBuff);

for (len = 0; len < lsize; len++) {
if ((ary[len] = GetCSV(strBuff)) == NULL)
break;
}
free(*strBuff);
return len;
}

strdupの後メモリを開放しているつもりなのですが問題あるのでしょうか？

C言語をあまりしたことがないので詳し方ご教示願えないでしょうか？宜しくお願いします。

回答No.2

hanabutako · ベストアンサー

> strdupの後メモリを開放しているつもりなのですが問題あるのでしょうか？
関係ないところをstrdupで確保したメモリ領域だと思ってfreeさせてますね。
GetCSV関数の中でif ((*pBuf = strpbrk(q,",")) != 0) *(*pBuf)++ = 0;という箇所がありますが、これによってsplit関数の*strBuffの値が書き換えられています。つまり、これが行われたあとにfree(*strBuff);してもメモリアロケータが知らないポインタをfreeしてほしいと申請しているわけなので、strdupで作られた領域はfreeされません。
正常にfreeしたいならstrdupしてからGetCSV関数を呼ぶまでの間に*strBuffのコピーをとっておく必要があります。

しかし、freeできるようになってもさらに問題があります。
ary[...]の指している値はstrdupしたメモリ領域を指しているので、strdupした領域をfreeしてしまうとaryの指している領域が無意味になってしまいます。多くの場合はメモリがすぐに再利用されないので問題に気づかないかもしれませんが、これはのちのちに見つけにくいバグになります。

そもそも、メモリの確保や開放をどこかの関数で勝手にやるとバグの温床になります。確保したところで開放するというのはよいとは思いますが、確保したメモリ領域を呼び出し元が使う場合はこの実装を避けた方がよいでしょう。つまり、splitの呼び元でstrdupしてからsplitの引数として渡す方がよいでしょう。
そうすると、例えばこんなプログラムになるでしょうか。

/*
* CAUTION: May change the argument 's'.
*/
int
split(char *ary[], const int ary_nelms, char *s)
{
int len = 0;

for (len = 0; len < ary_nelms; len++) {
if ((ary[len] = GetCSV(&s)) == NULL)
break;
}
return len;
}

int
main(void)
{
char str[] = "a,b,c,,e", *tmp, *ary[BUF_SIZE];
int i, len;
tmp = strdup(str);
len = split(ary, BUF_SIZE, tmp);
for (i = 0; i < len; i++)
printf("%s\n", ary[i]);
free(tmp);

return 0;
}

あるいは、もし、splitの引数が質問で示された形に決まっているとしたら、こんな感じで作るでしょうか。この場合、aryのサイズが小さすぎるとメモリを破壊しますし、split二度呼び出すと前回のsplitの結果が失われ、無意味なものになってしまいます。
static char *csv_string = NULL;
int
split(char *ary[], char *s)
{
int len, lsize;
char *csv_buff;

if (csv_string != NULL)
free(csv_string);

csv_string = csv_buff = strdup(s);
lsize = strlen(s);
for (len = 0; len < lsize; len++) {
if ((ary[len] = GetCSV(&csv_buff)) == NULL)
break;
}
return len;
}

自分だったら可能な限り前者の実装にしますね。

お礼

回答ありがとうございます。

丁寧な説明を記述いただいた上、ロジックまで記述いただきありがとうございました。

始めの案で処理したところメモリリークはしなくなりました。

大変助かりました、どうもありがとうございました。

回答No.3

kmee

> VB6でCSVをリードして処理しているのですが、処理速度が遅いため
> Ｉ／ＯをしているところをＣに変更して処理速度

その目的なら、まずはVBの方のプログラムを見直す方が効果的だと思います。
特に、質問にあるようなメモリの確保と解放を繰り返すようなものだと、 大きな効果は期待できません
(メモリの確保と解放は比較的時間のかかる処理です)

例えば
・VB6にsplitという関数があります。CSVの各要素の分解するのに有効です。
http://jeanne.wankuma.com/tips/vb6/string/split.html
・変数を全部Variant型にしてたりしませんか?

お礼

回答ありがとうございます。

Variant型にはしていないのですが、ＶＢの方も無駄な処理がないか調べて見ます。

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

回答No.1

asuncion

＞char*strBuff[BUF_SIZE];

ポインターの配列を定義していて、

＞*strBuff = strdup(s);
＞lsize = strlen(*strBuff);
＞free(*strBuff);

その配列の先頭要素だけをさわっている理由を教えてください。

お礼

ご指摘ありがとうございます。

色々試しているうちに意味のないロジックになっていました。

以後気をつけます。