プログラミング 間違いさがし

Question

この関数の定義には致命的な間違いがあるみたいなので間違いと解決方法を教えてください。

char *str_concat(char *s1, char *s2){
char s3[256];
char *p = s3;
while (*p++ = *s1++);
p -= 1;
while(*p++ = *s2++);
return s3;
}

Comments

[maiko0318]

何をしたくてこの関数を作ったのか仕様は書いてもらわないと間違いがわかりません。

[kakaadada]

文字列を結合して返す関数str_concatの間違え探しです

Answer 1

質問で言語が指定されていませんが、C++っぽいので、C++11以上だという前提で書きます。

C++11以上で文字列を扱う場合はchar *よりもstd::stringを使うべきであり、参照渡しやムーブセマンティクスを意識して書くべきです。

std::string str_concat(const std::string &s1, const std::string &s2)
{
    std::string s3;
    s3.append(s1);
    s3.append(s2);
    return s3;
}

つまり、C++を使っているのに、文字列操作に便利なstd::stringを使わずにchar *を使っているのがそもそもの間違いと思われます。最適化によって速度もそれほど変わらないはずですし、ローカル変数をそのまま返せばムーブセマンティクスになっているはずです。

Answer 2

ローカル変数として用意したs3を関数の返り値として返してしまうのはまずくないですかね?
代替案としてはmallocなどでメモリ領域を確保して、そこに書き込んで返す...などでしょうか。

Answer 3

中身と関数名を見る限り、s1 および s2 を結合した結果文字列を返す関数、のようです。

が、s3 が 256byte という固定長です。
s1 と s2 の合計長が 256byte を超えると、segfault しますね。

Answer 4

char *str_concat(char *s1, char *s2){
char s3[256];
char *p = s3;
while (*p++ = *s1++);
p -= 1;
while(*p++ = *s2++);
*p=0;                      //抜けているかも？
return s3;
}