文字列のポインタを使って、長さが短い順に連結させる

Question

英単語を入力し，それらを「長さが小さい順に」連結したひとつの文字列を作成し，画面に表示するプログラムを作成しています。しかし、うまく行きません。なぜなのでしょうか？

理想の結果が

英単語何個ですか？4
1個目r
1
r
2個目eee
3
eee
3個目wwwww
5
wwwww
4個目ii
ii
連結結果　：riieeewwwww

ですが、
自分のやつは

英単語何個ですか？4
1個目r
1
r
2個目eee
3
eee
3個目wwwww
5

4個目ii
2
ii
連結結果　：r???

です。

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

//英単語の長さ                                                                                       
int str_length(const char *s)
{
  int len = 0;

  while (*s++){
    len++;
  }
  return len;
}

int main(void)
{
  int i, j, k, l, m;
  int number; //英単語の個数                                                                         
  char eitanngo[number][34];
  int size[100];//英単語の長さ                                                                       
  int tmp;
  char TMP[34];


  printf("英単語何個ですか？");
    scanf ("%d", &number);
    for(j = 0; j < number; j++){
      printf("%d個目",j + 1);
      scanf("%s", eitanngo[j]);
      size[j] = str_length(eitanngo[j]); //英単語の長さ                                              
      printf("%d\n", size[j]);
      printf("%s\n", eitanngo[j]);
    }
    //文字の長さに並べ、連結                                                                         
    for(l = 0; l < number; l++){
      for(m = l+1; m < number; m++){
          if (size[l] > size[m]){
            tmp = size[l];
            size[l] = size[m];
            size[m] = tmp;
            strcpy(TMP,eitanngo[l]);
           strcpy(eitanngo[l], eitanngo[m]);
           strcpy(eitanngo[m], TMP);
          }
        }
    }

    printf ("連結結果　：");
    for(k= 0; k < number; k ++){
      printf("%s", eitanngo[k]);
    }
    printf("\n");
//sizeが短い順になってるかの確認
    //for(k = 0; k < number; k++){
      //printf("%d ", size[k]);
    //}
    printf("\n");
    return 0;
}

Answer 1

色々考慮して書き換えたらこうなりました。

#ifdef _WIN32
#ifdef __GNUC__ // GCC or Clang
// msvcrtのprintfではzuが使用できないため
#define __USE_MINGW_ANSI_STDIO 1
#elif _MSC_VER // VC++
// VLA無効
#define __STDC_NO_VLA__ 1
// strcpy等の警告無効(わかってやっている)
#define _CRT_SECURE_NO_WARNINGS 0
// 0サイズ配列が構造体に最後に来る場合の警告無効
#pragma warning(disable : 4200)
#endif // __GNUC__ or _MSC_VER
#endif // _WIN32

#include <stdbool.h>
#include <stddef.h>
#include <stdint.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>

#define BUFF_SIZE 1024

struct word {
	size_t len;
	char str[];
};

// 行の読み込み
char *get_line(void);
// 英単語の個数読み取り
size_t input_kosu(void);
// 英単語の読み取り
struct word *input_word(size_t no);
// 長さ比較
int word_len_compare(const void *a, const void *b);

int main(void)
{
	// 英単語の個数を取得する。
	size_t number = input_kosu();
	if (number == 0) {
		fprintf(stderr, "%s\n", "取得できませんでした。");
		exit(1);
	}

	// 英単語の配列
#ifdef __STDC_NO_VLA__
	struct word **words =
	    (struct word **)malloc(number * sizeof(struct word *));
	if (words == NULL) {
		fprintf(stdout, "%s\n", "メモリ割り当てに失敗しました。");
		exit(1);
	}
#else
	struct word *words[number];
#endif // __STDC_NO_VLA__

	// 繋げたときの長さ
	size_t total_len = 0;
	for (size_t i = 0; i < number; i++) {
		words[i] = input_word(i);
		if (words[i] == NULL) {
			fprintf(stderr, "%s\n", "取得できませんでした。");
			exit(1);
		}
		// ついでに長さを追加
		total_len += words[i]->len;
	}

	// 長さ順にソート(安定ソートではない)
	qsort(words, number, sizeof(words[0]), word_len_compare);

	// 連結した文字列
	char *renketsu = (char *)malloc(total_len + 1);
	renketsu[0] = '\0';
	for (size_t i = 0; i < number; i++) {
		strcat(renketsu, words[i]->str);
		// ついでに解放
		free(words[i]);
	}
	printf("%s: %s\n", "連結結果", renketsu);
	free(renketsu);
#ifdef __STDC_NO_VLA__
	free(words);
#endif // __STDC_NO_VLA__

	return 0;
}

char *get_line(void)
{
	char *line = NULL;
	size_t line_size = BUFF_SIZE;
	size_t ptr_size = 0;
	while (line_size <= SIZE_MAX) {
		char *new_line = (char *)realloc(line, line_size);
		if (new_line == NULL) goto error;
		line = new_line;
		char *ptr = line + ptr_size;
		// line_size - ptr_size が intの範囲を超えることはないはず。
		char *result = fgets(ptr, (int)(line_size - ptr_size), stdin);
		if (result == NULL) {
			if (line == ptr) goto error;
			return line;
		}
		size_t read_size = strlen(ptr);
		if (read_size == 0) return line;
		if (ptr[read_size - 1] == '\n') return line;
		// next
		ptr_size += read_size;
		line_size += BUFF_SIZE;
	}
error:
	free(line);
	return NULL;
}

size_t input_kosu(void)
{
	while (true) {
		printf("%s: ", "英単語何個ですか？");
		fflush(stdout);
		char *line = get_line();
		if (line == NULL) return 0;
		char **line_end = NULL;
		long long num = strtoll(line, line_end, 10);
		free(line);
		if (0 < num && num <= SIZE_MAX) {
			return (size_t)num;
		}
		printf("%s\n", "1以上の正の整数を入力してください。");
	}
}

struct word *input_word(size_t no)
{
	while (true) {
		printf("%zu%s: ", no, "個目");
		fflush(stdout);
		char *line = get_line();
		if (line == NULL) return NULL;
		// 単語にするために空白や改行で区切る
		for (char *ptr = line; *ptr != '\0'; ptr++) {
			if (strchr(" \t\r\n\f\v", *ptr) != NULL) {
				*ptr = '\0';
				break;
			}
		}
		size_t len = strlen(line);
		if (len == 0) {
			free(line);
			continue;
		}
		printf("%zu\n", len);
		printf("%s\n", line);
		struct word *word_p =
		    (struct word *)malloc(sizeof(struct word) + len + 1);
		if (word_p == NULL) {
			free(line);
			return NULL;
		}
		word_p->len = len;
		word_p->str[0] = '\0';
		strcpy(word_p->str, line);
		free(line);
		return word_p;
	}
}

int word_len_compare(const void *a, const void *b)
{
	const struct word *a_w = *(const struct word *const *)a;
	const struct word *b_w = *(const struct word *const *)b;
	ptrdiff_t diff = a_w->len - b_w->len;
	// diffはintの幅を超える可能性が一応ある、たぶん
	if (diff > 0) {
		return 1;
	} else if (diff < 0) {
		return -1;
	} else {
		return 0;
	}
}

Visual C++(Visual Studioの一部としてインストール)、MinGW-w64、Clang(Windows)、BCC32C、WSL上UbuntuのGCCで動作確認しています。GCCやClangでは"-std=c11"を付けてください。それぞれ最新バージョンです。特に古いVC++では未対応のためエラーになる場合があります。

1. VLAは使える環境と使えない環境がある。
   VLAはC11ではオプション機能であり、環境によって使えません。C11準拠のコンパイラでは__STDC_NO_VLA__が有るかどうかで判断してください。ただし、Visual C++はVLAが使えないにもかかわらず__STDC_NO_VLA__は設定されていません(これはVisual C++がC11に準拠していないからです)。自分で足しておいてください。
   VLAが使えない環境では素直にmalloc()等でメモリを確保しましょう。free()を忘れずに。
2. scanf()で数値を読み取る場合は、整数型の範囲を超える場合に動作が未定義である(ここら辺は実はよくわかっていない、仕様としてはstrtol()と同じように動作と書いてあったりもする)という問題があります。strtoll()など型の範囲外の場合でも動作が未定義とならない関数で取得してください。
   今回は面倒だったのでget_lineで1行全てを取得するようにしています。
3. scanf()で文字列を読み取る場合は、サイズを指定、つまり、"%s"ではなく"%256s"のように取得する大きさを必ず指定しなければなりません。"%s"のような書き方はgetsと同じで、バッファオーバーフローを防ぐ方法がありません。
   ※ 取得サイズを別途指定するscanf_s()(オプション)を使用する方法もあります。
4. アルゴリズムに拘りがなければソートにはqsort()を使いましょう。ただ、安定ソートとは限らないため、安定ソートである必要がある場合は一工夫必要になります。
5. 配列のサイズにはsize_tを使いましょう。ただ、printf()で出力は%zuになるのですが、MinGW GCCが標準で利用するC標準ライブラリ(Windows標準のmsvcrt)や古いVisual C++では対応していません。__USE_MINGW_ANSI_STDIOでC標準に準拠したprint()を使うようにしてください。
6. 英単語の構造体を作って長さを入れているのは長さを英単語と一緒に保存するためです。strlen()はO(n)必要になるため、呼び出し回数が多くなると重くなります。特に、ソートの中で呼び出してしまうと、顕著な速度低下を招きます。
7. 構造体の最後が大きさ0の配列(大きさ未指定)にする機能はCの標準機能なのですが、Visual C++はMicrosoftの独自拡張だと警告を出します。標準機能のはずなのですが、よくわかりません。
8. 色々チェックしていないところがあります(途中で面倒になった)。安全性を見込むならassert等で真面目にチェックする必要があるでしょう。
9. 適度に関数に分けてください。上のコード例ですら、分け方が足りないと思っています。

Comments

[pepperleaf]

ご苦労さまです。
普段、ここまで注意して使ってないので参考になります。
ただ、質問者さんに取っては消化不良になりそうな予感。(個人の感想)

[yumetodo]

私の回答の「C言語で安全に標準入力から数値を取得」に書きましたが、scanfもatoi系もstrtol系を呼び出します。しかし、エラーハンドリングを行わないので使用してはだめです。errnoの値とendptrの2つを確認することが必要です。

[yumetodo]

私の回答の「C言語で安全に標準入力から数値を取得」に書きましたが、scanfもatoi系もstrtol系を呼び出します。しかし、エラーハンドリングを行わないので使用してはだめです。errnoの値とendptrの2つを確認することが必要です。

[raccy]

strtol系はエラーや数字でないなら0、アンダーフローやオーバーフローならLONG_MINやLONG_MAXを返すので、0より大きくSIZE_MAX以下をチェックしているのであれば、errnoのチェックまではしなくてもいいかと思っています。

Answer 2

  int number; //英単語の個数                                                                         
  char eitanngo[number][34];

numberが未初期化なため、eitanngoをVLAとして作れません。大きさがわからないので。

prog.c:19:3: warning: 'number' is used uninitialized in this function [-Wuninitialized]
   char eitanngo[number][34];
   ^~~~

それはそうと、scanf系関数で数値の入力を受けてはいけません。

C言語で安全に標準入力から数値を取得

Comments

[pepperleaf]

VC++ 2017 だと、 "error C2057" でコンパイル通りませんでした。
> char eitanngo[100][34];
だと期待する結果に一致。

[yumetodo]

>VC++ 2017 だと

Visual StudioはC99のVLAに対応していません。通らないのは当然。

[yumetodo]

じゃなくてscanf ("%d", &number);のあとに定義しないとだめだよね。

[pepperleaf]

質問者さんの環境が不明なので、VLA　と言われても困る。
それにそんな事まで、元のコードは考慮している?

[yumetodo]

あきらかに元のコードはC99のVariable Length Arrayを使おうとしていますよね？

[Eki]

分かる人から見ると VLA になっていることは分かりますが、質問者さんはそこまでは考えず単に number 個の配列を作りたいなーくらいの感覚なんだろうとだと思います (つまり VLA が普通の配列とは少し違うものだという認識はおそらくないはずです) 。
質問者さんがその後 Visual Studio を使われることがあれば別のエラーを誘発して困られたかもしれませんし、例えばリアルで使用が認められない環境にいる (ex: 大学の授業で教師が認めない) 場合もあると思うので、一言補足があってもよかったのかなとは思います（どこまで回答者がすべきかというところもあるので、あくまで個人的な意見です）。

Answer 3

うまく行きません。なぜなのでしょうか？

あなたのコードにバグがあるから。

というのはともかく、

int str_length(const char *s)

これはstrlen という標準関数があります

int number; //英単語の個数
char eitanngo[number][34];

こういう定義はできません。そもそもコンパイルエラーになるはずですが

  scanf("%s", eitanngo[j]);

入力文字数が３３文字以上あった場合、このコードは破綻します

scanf関数は使用禁止としましょう
業務に使えないコードを書くのは無駄でしかないですぜ

Comments

[raccy]

配列の大きさに変数を使うのはVLAというC99(標準)およびC11(オプション)の機能です。Visual C++のようにC++がサポートする範囲のC99(全体の一部)にしかCについては準拠していないコンパイラでは対応していませんが、GCCやClangのようにC99やC11に準拠したコンパイラではVLAに対応しています。ですので、そのようなコンパイラを使えばコンパイルエラーにはなりません。yumetodoさんが言うように未初期化の変数利用による警告(エラーではない)が出るだけです。

[y_waiwai]

ワーニングなんだから気にしなくてOKOK。っていうわけにもいかないですわな。

どっかのコンパイラがエラー出さないからという理由で、これはエラーではない、と強弁するのは考えものですぜ。

[yumetodo]

warningはすべて確認して不必要なら#pragmaやattributeで部分的に消す。という重要性が改めて。