Q&A
不特定多数の人にレビューしてもらうというのはなかなか難しいと思いますが、GitHub の URL を教えていただければ見てみますよ。自分は暗号そのものは詳しくないので、C のコードの書き方になりますが。
前提・実現したいこと
いま暗号を実装しています。だいたい出来上がったのでGitHubにソースコードを載せているのですが、
コードレビューの方法がわかりません。
コードを第３社に見てもらうためにはどうすればいいのでしょうか？
開発言語はLinuxのC言語です。
またプログラミングする上で、気をつけなければならない基本的なことがあったら教えてください。
コードに関する質問でないですが、この質問は不適切でしょうか？
よろしくお願いします。
GitHubのアドレスはここです。
https://github.com/anang0g0/GoppaDecorder
この中のoplib.cがメインの関数です。コメントの付け方とか、基本的なコードの書き方とかでもいいです。
このコードは、以下のサイトと関係しています。方式はMcEliece暗号です。
https://csrc.nist.gov/projects/post-quantum-cryptography/round-2-submissions
https://github.com/anang0g0/GoppaDecorder
こちらになります。メインのファイルはoplib.cです。
よろしくお願いします。
Goppa符号…、数学ですね。頑張ります。
https://ja.wikipedia.org/wiki/%E3%82%B4%E3%83%83%E3%83%91%E7%AC%A6%E5%8F%B7
自分はどちらかというと科学計算にしか興味がないので、応用数学とかが好きなんですよね。ちなみに自分はプロのエンジニアではありません。参考文献はすべて英語の論文を読みました。Wikiのリンク先の解説を読みましたが、ここで質問しているのは１変数の場合です。古典的語っぱ符号と言われています。幾何学的Goppa符号もあるのですが、それはまた別のリポジトリにあります。
https://en.wikipedia.org/wiki/Binary_Goppa_code
こっちの方ですね。
oplib.c をコンパイルして動かしてみました。H.key はできましたが、その後残念ながらクラッシュしてしまいました。これって特に入力ファイルとか指定しなくていいんですよね？
一緒に使うインクルードファイルの設定が違うかもしれないのでもう一度確認してからお知らせします。
コアダンプですか？git cloneして動作確認しましたがこちらではエラーは出ませんでした。
ちなみにこちらの実行環境はDebian９ strechです。こちらの環境でうまく行っても、他の環境で動くかどうかはわからないのでそのへんも確認したいです。H.keyができているので、１８５７行目まではうまく行っているんだと思います。同様のクラッシュを確認しました。gcc -g オプションでデバッグしています。-O2オプションをつけると正常に動作するのですが、現在確認中です。
どうやらxgcdのバグのようですが、よくわかりません。とりあえず-O2オプションをつければ問題なく動くと思うのですが試してみてもらえますか？
こちらは Ubuntu 18.04 LTS です。-O2 オプションをつけたら完走しました。が、私の不勉強ゆえ何をしてるのやらさっぱり…。
基本的な質問で申し訳ないんですが、Binary Goppa 符号というのは、エラー訂正できる暗号系（？）ってことなんでしょうか？ 具体的に、何ができれば（どういう入力に対してどんな計算結果になれば）期待通りに動いてることになるのでしょうか？
この質問の本題はどれでしょうか？（どれが解決すると解決済みとなる質問でしょうか？）
(1) コードを第３社に見てもらうためにはどうすればいいのでしょうか？
(2) 追伸：次のコードにバグがあり、クラッシュしてしまいます。・・・
(3) コードレビューしてください
hishi様。
O2オプションをつけないでコンパイルするとたしかに暴走しました。まだ特定できませんが、コードに何か問題があるのだと思います。でもとりあえず最適化オプションをつければ動くので今まで気にしていませんでした。whileループで延々と動くようにしてあるのは、鍵やエラーを変化させても暴走しないことをチェックするためです。２４ｈ動かしても止まらなかったので、バグはないと思って今回のコードレビューに踏み切りました。バイナリGoppaは誤り訂正符号という無線でデータ通信を行うときに使う符号化技術です。その符号をさらに公開鍵暗号に応用したものが私のコードです。McEliece暗号で検索すると解ると思います。
Y.H様。
最初はレビューしてほしくて投稿をしたのですが、いざレビューが始まると指摘された問題点を解決するために更に書くことが増えてしまいました。どうすればいいのでしょうか？コードのクラッシュも聞きたいですが、これは別の質問に回したほうが良さそうですね。コードレビューが目的です。そのためにどうすればいいかが本題です。
なるほど、公開鍵暗号なんですね。ところで、Teratail での質問はどうしても一問一答という感じになってしまうので、コードレビューにはあまり向いてないと思います。私のレビューは（ある程度理論を理解したいので時間がかかると思いますが）GitHub の issue の方に書かせていただこうかと思います。
回答3件
0
ベストアンサー
以下、可読性重視でコメントしますね。
- 先ず１ソースが長すぎます、分割しましょう。
- defineのマクロ名はせめて単語レベルで記述した方が良いと思います。（変数名も同様）
- 外部変数が多いのも気になります。getter/setter関数を作成して隠蔽化をお勧めします。
- 四則演算は演算子の優先順位に頼らずカッコで順序を明示しましょう。
例）99行目
while(count<K/2-1){
↓
while(count < ((k / 2) - 1)) {
※正直コードそのものが暗号化しちゃっていて追うのが辛いです。。。
問題のクラッシュですが、具体的にはどんなエラーとなるのでしょうか？
そこそこ大きなサイズのローカル変数があるようなのでstack overflowとか？
投稿2020/02/26 00:24
総合スコア1095
１．分割コンパイルの方法がわかりません。何を基準に分けたらいいのかもわかりません。分割した場合Makefleを書かないといけないと思うのですが書いたことがありません。本を買って勉強します。
２．defineは超適当なので名前付けも兼ねて可読性を上げるように直したいと思います。
３．外部変数が多いのは、行列や配列を戻り値に指定出来ないからで、もしかするとポインタ渡しで解決できるかもしれません。
クラッシュについて：本当に不思議なのですが、１０７６行目のxgcdという関数でコアダンプします。関数の内部でエラーが起きるのだろうと思って、最後の方にexit関数を入れて強制終了させるのですが、そうするとバグは起きません。なので具体的にどこが暴走する原因なのかつかめなくてお手上げです。
１．分割について
typedef定義はヘッダー化してincludeする。(仮に hogehoge.hとする)
外部変数は別ソース化して、getter/setter関数を作成する。
分割についてですが、先ずはビジネスロジック（「ランダム多項式の生成」や「ランダム置換の生成」）と、共通処理（P2Mat, b2B等）を別ファイル化する。
２．可読性について
これは本当に重要です、自分がもしこの命名でコードを書いたら１か月後に自分自身解読に相当な時間が掛かります。
３．外部変数について
お気づきの通りポインタで解決できます。
またコアダンプについてはgdbを使えばどのコードでコアダンプしたか追跡できます。
デバッグでは基本となることなので、ネット上に事例がたくさんあると思います。
高価な専門書を購入しなくても、ネット上に有益な事例がたくさんあるので、がんばって学習してください。
Makefileはmkmf等の自動生成ツールを使うと楽に書けます。
Dream様。
ありがとうございます。こういう意見は貴重です。クラッシュの件ですが、構造体の配列を初期化出来ずにクラッシュしていたようで、mallocとmemsetを使ってメモリの確保と初期化を行ったら無事動くようになりました。自己解決です。可読性ですが、まずコメントを入れてみようと思います。コメントの書き方がわかれば教えてください。
0
・インデントが出来ていません.
・{} の付け方, スペースの入れ方, 空行の入れ方が一貫していません.
これだけでコードレビューはNG判定され, 次のレビュー段階には進めなかったりします.
IDE を使っていれば即直せるのですから, やっていないことを言われますし, 使っていなければ, 修正に時間がかかることですから最初からやれと言われます.
投稿2020/02/26 15:04
総合スコア13209
ありがとうございます。自分はemacs使っているのですが、インデントにはあまり気を配りませんでした。emacsなので誰かファイルを開けたときに自動的にインデントを揃えてくれるLispマクロを作ってくれないかなーｗ。
Goldwasserさん
linux環境ならindentコマンドがあるはずです。
厳しい指摘と思われているかもしれませんが、第三者にコードレビューをお願いする場合、インデントを整えるのは最低限のマナーですよ。
そういうのがあるんですね。ありがとうございます。
あ、でもこれ私のスタイルと違う・・・。自分はemacsを使うので、関数の戻り値と関数名と括弧を同じ行に書いて、括弧の閉じ忘れがないように書くんですが、indentコマンドを使うとそれが解らなくなってしまうのでそういう書き方にしているわけです。インデントは必ず治すのでお手柔らかにお願いします。
emacs にはデフォルトで（？） cc-mode があって、インデントをしてくれてますが。
やり方は、コードの領域を選択しタブキーを押すだけ。
私は ubuntu の emacs を常用していて、そこは特に設定した覚えがない。
わかります。emacs便利ですよね。背景をcodeみたいに黒くできればいいんですけどね。
> でもこれ私のスタイルと違う
コードの書き方は流派(?)があって、C言語でもBSDスタイル、GNUスタイルなど様々なものがあります。詳細は「C言語 字下げスタイル」などで検索してみると良いと思います。indentやastyleなどのフォーマッターと呼ばれる類のツールは、そうしたスタイルのカスタマイズもできます。
コードレビューにおいて空白の有り無し、字下げなど見た目の指摘は些末なことに思われるかもしれませんが、指摘する方、読む方にとっても実は意外とストレスです。細かい指摘にうんざりするようであれば「アルゴリズム」とか「実装方法」とか、レビュー対象の優先度を明示すれば良いでしょう。それでもコードの見た目の印象で信頼性を疑われてしまうことはあるので、本質部分に自信があるのであれば、尚更、それらをおろそかにしない方が良いです。
> 背景をcodeみたいに黒くできれば
$ emacs --reverse &
dodo様。
内容にもまだ自身がありません。とりあえずうこくようになったというだけで、完成している自身がありません。しかも正しく理解しているかというとそういうわけでもなく、計算に失敗する場合新しい鍵を取り直すという方法で逃げています。確かにsagemathみたいな計算ソフトで検証して、エラーがでる場合、逆元が計算できなかったりする多項式があることは確認済みですが、どうやれば正しい鍵だけを取ってこれるのか、まだ分っていません。論文によると、既約多項式の場合に限定しているのでそれだけを鍵に取るのであれば問題がないのかと思います。既約多項式の判定法は調査中です。
２４時間動かしてエラーが出ないことを確認した程度で、それがコードの品質を保証する手段だとは思えません。自分は製品のデバッグ方法を知らないのです。
rubaro様。
ありがとうございます。
BSD/KNFスタイル
自分はこれですね。indent で指定するにはどうすればいいんでしょう？
いいや、わからないから手で直そうｗ
> indent で指定するにはどうすればいいんでしょう？
ご自分で色々試してみてください。必ずしも完全にご要望のかたちになるとは限りません。使い方はLinuxであれば manコマンドやinfoコマンドで詳細が分かります。$ man indent、 $ info indent など。
https://linuxjm.osdn.jp/html/GNU_indent/man1/indent.1.html
indent で要望を満たせないなら、別のツールを使う必要があるでしょう。
＞ Goldwasserさん
> 内容にもまだ自身がありません。とりあえずうこくようになったというだけで、完成している自身がありません。
私自身は本プログラムの背景にある理論を理解する前提知識が無いので回答を避けたのですが、jimbeさんの回答にあることは同様に思ったので、コメントしたまででした。ご自身で内容に自信がないのであれば、プログラムが抜かりなく堅牢に作られていることを、プログラム自身で保証するように漸進的に作り上げていくことをおすすめします。ユニットテスト（単体テスト）の手法です。（詳細は別途調べてみてください）また、広く使われているオープンソースの製品ではその提供するソースコード自体にテストコードが組み込まれていることが多いです。セットアップの際にプログラム自身で既知のテストパターンを実行し、その環境で正しく動くのか自動でテストします。これらはソースコード自体が「その機能を保証している」と言う何よりの証拠になります。また、やってみて初めて気が付くのですが、これら自動テストの類は、その後の機能追加や修正でのストレスが劇的に減ります。
> ２４時間動かしてエラーが出ないことを確認した程度で、それがコードの品質を保証する手段だとは思えません。
そうですね。大抵の製品で耐久テストは実施されますが「やりました」と言って終わりです。第三者によるテストパターンの妥当性の検証も再現も困難なので、残念ながらアピールポイントにはなりません。
> 自分は製品のデバッグ方法を知らないのです。
では、知りましょう。それだけです。
誤解していただきたくないのは、コードのスタイルや製品の品質に拘らなければいけない訳では"ない"と言うことです。（＜これは賛否両論あるかもしれません。私自身の意見です）個人の趣味、自分の愉しみのためだけなら、コードレビューでわざわざ嫌なことを指摘されるまでもないでしょう。反対に、例えばオープンソースやその他販売製品として広く世に問いかけ、使われたいと言うのであればそうはいかない、と言うことです。
jimbeさんの回答コメント欄で長々と失礼しました。以降は控えます。
> jimbeさんの回答コメント欄で長々と失礼しました
いえ, teratail 自体オープンな場と考えておりますので, 私としましてはコメント頂くことは全く失礼はありません.
返ってご配慮頂いて恐縮です. ありがとうございます.
テストコードを書くというのはやったことがないですが、そのためにはまず答えを用意する必要がありますね。インデントについては自分のやり方を定式化して把握することが最初にやることだと思うのでがんばります。でもそんなにいっぺんには出来ないので少しずつですけどね。今のところ正しい答えがでる場合があるという程度なので、完成にはほど遠いです。
人に見せるときはインデントを整えるというのが勉強になりました。
0
多数のOP構造体変数がスタック領域に取られることから、スタックオーバーフローが起こったようです。
OP型構造体変数のサイズは3072バイトです。それがローカル変数としてスタック上にいくつもとられ、pattarson(), xgdb() 関数の引数にもなっています。他にもサイズの大きな変数が見つかります。コード全体で変数の設計を考えなおす必要があります。
-O2 最適化した場合は、ローカル変数の使用量を減らす最適化がおこなわれるようです。
oplib.c のみコンパイルして現象を再現できるようですが、コンパイルと起動方法、デバッグ操作が合ってるか、ご確認ください。ただしスタックオーバーフローは実行環境が異なれば、発生する場所が異なる可能性があると思います。以下は私の実行例です。
bash
1$ cc oplib.c -g 2oplib.c: In function ‘add’: 3oplib.c:404:9: warning: format ‘%d’ expects argument of type ‘int’, but argument 2 has type ‘long long unsigned int’ [-Wformat=] 4 printf("n1=%d\n",n1); <<= 無視して構わない警告が数件、省略 5 6$ gdb ./a.out 7# gdb の開始メッセージが10数行、省略 8Reading symbols from ./a.out...done. 9(gdb) r <<= 実行開始 10 11# 膨大な表示、省略 12963x^20+2839x^19+7684x^18+7501x^17+4141x^16+4568x^15+7852x^14+532x^13+326x^12+3373x^11+6021x^10+5577x^9+2960x^8+660x^7+7390x^6+2625x^5+1835x^4+4297x^3+3995x^2+4073x^1+6692 g1!========= 13DEBUG: point 1. <<= 追加したデバッグ表示 14 15Program received signal SIGSEGV, Segmentation fault. 160x00000000004086e6 in xgcd (f=..., g=...) at oplib.c:1013 17warning: Source file is more recent than executable. 181013 { 19(gdb) l 201008 // exit(1); 211009 return h; 221010 } 231011 241012 EX xgcd(OP f,OP g) 251013 { 261014 printf("DEBUG: f: %p, g: %p\n", &f, &g); 271015 281016 OP h={0},ww={0},v[K*2]={0},u[K*2]={0}; 291017 oterm a,b; 30(gdb) bt 31#0 0x00000000004086e6 in xgcd (f=..., g=...) at oplib.c:1013 32#1 0x000000000040c570 in pattarson (w=..., f=...) at oplib.c:1626 33#2 0x000000000040efdb in main () at oplib.c:2098 34(gdb) 35(gdb) p f 36$1 = {t = {{n = 0, a = 1}, {n = 1, a = 1}, {n = 0, a = 0}, {n = 3, a = 1}, { 37 n = 4, a = 1}, {n = 0, a = 0}, {n = 0, a = 0}, {n = 0, a = 0}, {n = 8, 38# OP f 変数のサイズが大きいため表示行多し、省略 39 n = 101, a = 1}, {n = 0, a = 0}, {n = 0, a = 0}, {n = 104, a = 1}, { 40---Type <return> to continue, or q <return> to quit---
main() => pattarson() => xgcd() と関数呼出しが行われ、xgcd() 関数の先頭でSegmentation Fault を起こしたように見えます。私は xgcd() 関数の先頭にも printf() でDEBUG表示を追加しましたが、その表示はありませんので、 xgcd() 関数内部の問題ではなさそうです。
デバッグのため、pattarson() 関数の中にも printf() 行を追加してあります。
C
1void pattarson(OP w,OP f) 2{ 3 OP g1={0},ll={0},s={0}; 4 int i,j,k,l,c; 5 /// 省略 6 printf(" g1!=========\n"); 7 printf("DEBUG: point 1.\n"); // ここを通過直後 8 9 if(LT(g1).n==0 && LT(g1).a==0){ 10 printf("DEBUG: point 2.\n"); // ここは通らない 11 12 printpol(o2v(w)); 13 printf(" badkey=========\n"); 14 exit(1); 15 } 16 hh = xgcd(w, g1); // ここで SIGSEGV? 17
コードレビューする側としてコメントしたいことはありますが、取り急ぎ問題点と思われる点を回答します。お気づきのように、既にコードに手を入れて、少しづつ私のスタイルに変更しつつあります。
#スタックオーバーフローによってSegmentation Faultが起こった事は確実
使ってないOP構造体がたくさんありすぎてメモリが溢れた
その結果、
-O2 最適化した場合は、ローカル変数の使用量を減らす最適化がおこなわれる
スタックオーバーフロー直前のスタックトップ付近のアドレスと、main() が動作開始した時点のスタックポインタ（付近）の差をとれば、どれだけスタック領域を消費したかがわかります。手元で調べたところ、約7.8MB消費したことがわかりました。xgcd() もスタックに巨大なローカル変数（v[K2], u[K2]等）を取りますので、それらを初期化しようとしたところで落ちたのでしょう。
一方、-O2オプションでコンパイルすると未使用な変数は削除され、スタック領域の消費量が減り、3.8MB程度に半減したことがわかりました。
１６Gもメモリを積んでいるので暴走しないはず
各プロセスは実装メモリを独り占めすることはできません。ＯＳ（Linux, Windows等）はメモリ資源を管理しており、プロセスには実装メモリの一部分しか与えないからです。
実行時スタックサイズ変更 on Linuxを見ると、デフォルトは8192であり、これは8MBがプロセスに与えられることを意味します。約7.8MB消費した時点でスタックは溢れる寸前だったわけです。このページに倣って、16MBのメモリを与えると最適化しないプログラムも動作を続けることができました。即ち、スタック領域が足りなかったことは確実です。
bash
1$ ulimit -s 16384 <<= 16MB に拡張する 2$ ./a.out <<= 動作できる
コードレビューしてみます。
- 不要な変数は削除しましょう。pattarson() 関数の中をざっと見ただけですが、
unsigned short m[K],mm[T]={0},dd[K*D]={0}; // <<= すべて不要 OP h={0},r={0},aa[K]={0},tt={0},ff={0}; // aa[k] が不要
OP aa[K]; だけで 786KB (== 3072 * 256)を消費します。こうした未使用の変数を削除するには全ての警告(warning)を出させるオプション -Wallを指定するとよいです。
$ cc oplib.c -Wall
- 不要な変数を、各関数に同じようにとっているかのようにも見えます。そこから、変数のスコープを理解していないのではないか、各関数の役割を整理しないままコーディングしているのではないか、といったことを疑います。
- コードが記述されたファイルをインクルードするのはイリーガル。インクルードするのはヘッダファイルにするのが普通。ヘッダファイルには定義を書き、コードは書かない。
#include "chash.cpp" // インクルードしてはいけない。分割コンパイルせよ #include "lu.c"
一番簡単な分割コンパイルのやり方は次（他にも修正が必要になりそうなので私は試してない）。
$ cc oplib.c chash.cpp lu.c
- 計算式で定義する定数マクロはカッコで囲むのが安全。数字をカッコで囲む人もいる。
#define K (128*2) // こうしたマクロはカッコで守ろう #define DEG (3*K) #define T (K/2) #define E (13) #define D 6688 // こういう値の意味・根拠は？
- K, T, E, D, c[2 * K + 1], g[K + 1] ・・・このように文字数の少ない識別子が大手を振って使われるのは、よくありません。スコープが狭い、或いは寿命が短い変数であれば構いませんけど。
- 構造体のメンバ名が短いのは、構わないけど、何か説明のコメントが欲しくなります。
typedef struct { unsigned short n; // 何かコメントを書こう unsigned short a; } oterm;
- deg()関数はreturn値が不定となる可能性あり。-Wallは警告するかもしれない。
int deg(vec a) { // 省略 if (n > 0) return n; // 警告！！！nが０なら戻り値は不定？ }
if (n > 0) という条件判定は不要で、単に return n; すれば良いのではないか。
-
構造体を返す関数に注意。o2v(), v2o(), init_op()等は、ポインタを受け取って、そこに結果を書き込んだほうが効率が良いのだけどなあ・・・
-
OP oadd(OP f, OP g) も、上記のようにポインタを受け取りそこに結果を書き込むこともできるが、今の仕様のままでもツッコミどころがいくつか。初期化が不要な変数だってある、初期化だけでも時間はかかる。
OP oadd(OP f, OP g) { #ifdef ORIGIN vec a = {0}, b = {0}, c = {0}; // a, b は初期化不要 #else vec a, b, c = {0}; // c は０クリアしたほうが安全か #endif int i, k; OP h = {0}; // 不要。下の return を見よ a = o2v(f); // ここで a, b に値が代入される。０クリアする必要無し b = o2v(g); #ifdef ORIGIN // 元のコード // 実行時、deg() を３回呼び出すが if(deg(a) >= deg(b)){ k=deg(a)+1; }else{ k=deg(b)+1; } #else // 改良コード int ka = deg(a); // 一時変数に取れば、呼出しは２回で済む int kb = deg(b); if (ka >= kb) { k = ka + 1; } else { k = kb + 1; } #endif // c を０で初期化しておく必要はあるか？ 不明 for (i = 0; i < k; i++) c.x[i] = a.x[i] ^ b.x[i]; // ここで c は x[k-1] まで決定する #ifdef ORIGIN h = v2o(c); return h; #else return v2o(c); // 元の２行は１行で書けるので h は不要 #endif }
- ループの中で構造体変数 f の値は変化しないので、terms(f) は一回計算するだけで良い。
int odeg(OP f) { int i, j = 0, k; #ifdef ORIGIN for (i = 0; i < terms(f) + 1; i++) { // terms(f) を毎回計算するの？ #else k = terms(f) + 1; // 計算は一回だけ for (i = 0; i < k; i++) { #endif
そもそも引数の f は、構造体そのものを引数にするのではなく、元の構造体へのポインタ（アドレス）だけ受け取れば十分なのではないか。構造体を引数にすると、実引数から仮引数へコピーが毎回発生するので効率悪い。
（もう寝るｗ）
さらに追加します。
-
実際、不要なローカル変数が山のよう・・・
-
関数の最後にあるべき return 文が無く、関数の最後で目的の値を返さない経路がある。既に、上で int deg(vec a) 関数を指摘したが、他にもあった。コード品質の低さを示すもの。
unsigned short oinv(unsigned short a) // BUG !!!
unsigned char chk(OP f)
int isqrt(unsigned short u)
OP osqrt(OP f, OP w) --- 長大な関数
- 信用を失う滅茶苦茶な関数w
C
1OP ToHorner(OP f) 2{ 3 vec v = o2v(f); // この計算は何？ 4 OP h; 5 return h; // 不定値を返す OMG 6}
- 変数に代入するが使わない。何をするつもりか意図が不明な処理が多数見つかる。
C
1// 計算結果を使わない例1 2int main(void) // argc, argv を使わない 3{ 4 unsigned short mm[T]; // mm[t] = {0} 初期化不要 5 // 途中省略 6 for (i = 0; i < T; i++) { 7 mm[i] = r.t[i].a; // mm[i] に代入するも、使わない 8 9// 計算結果を使わない例2 10OP ogcd(OP f, OP g) // ユークリッドの互除法? 11{ 12 OP h, ww; // 初期化不要 13 14 for (int i = 0; i < T; i++) { 15 h = omod(f, g); 16 ww = odiv(f, g); // この除算は不要 17 f = g; 18 g = h; 19 } 20// 一方、xgcd() は除算した結果 ww を使う。そこをコピペしたか？
lu.c を拝見しました。
- 一文字のグローバル変数が！言語道断モノですw
C
1int i, j, k; // カウンタ !!! OMG !!! 2int n = F; // 配列の次数 ??? for fwrite()
ここをコメントアウトすると、これらに頼っているコードが判明します。
この小文字変数 k に頼るコードが det() 関数に見つかります。
C
1void det(unsigned short g[]) 2{ 3 // 省略 4 k = cc[K]; 5 // 途中省略 6 cc[K] = k;
この大文字「K」は #define K 128*2 という定数マクロです。大文字・小文字のK, kを一緒に使うのは、いかがなものかと（乱視が悪化してきた私は）思う。
i, j, k は、頼っている関数にローカル変数を追加すれば削除できます。
問題は n です。n の主目的は fwrite(dd, 1, n, fq); の引数とみられますが、他の箇所から n の値を変更可能です。たとえば oplib.c に scanf("%d", &n); が何箇所かあり、（意図通りかどうかは不明だが）キー入力した値がこの n に読み込まれるらしい。グローバル変数に対する典型的な戒めを変数「n」に見出すことができます。
しかも「n」という変数名は、関数内のローカル変数にもあるうえに、oterm 構造体のメンバ変数でもある。カオスです。
グローバル変数、マクロ名、寿命の長い変数、アルゴリズム的・意味的に重要な変数などはもっと長い変数名にしたほうが良いでしょう。ひとつの目安は grep コマンドで、それぞれの変数が登場する箇所を特定できること、でどうですか。
- chash.cpp - 拡張子 cpp は C++ ソースを意味するが、実際は C 。
C
1void SHA512_transform(unsigned long long H[], unsigned long long W[]) 2{ 3 static const unsigned long long K[80] = { 4 0x428a2f98d728ae22ULL, 0x7137449123ef65cdULL,
K[] という配列名は #define K 128*2 と被ります。コードの可読性・保守性の観点から、両方の「K」という名前をそれぞれ変更したほうが良いと思います。
・・・と、これだけでも、他の人が使えるコード品質になるには道のり長そうですね。
Enjoy !
scanfを入れているのは、表示を確認するために一時停止をしたいから
そのためにグローバル変数に読み込むのは本末転倒・言語道断。その後 fwrite() はどうなるか考えよ。
scanf() で停止するなら <stdio.h> の入力関数 fgets() 等でも停まります。getchar(); とだけ書いたこともあるけど、普通は小さな関数を作って、停めたい所で呼ぶでしょう。
C
1// extern int n; ← ＮＧ！ 現状、こうなっている 2void wait4keyinput(void) 3{ 4 int n; // 読み込む変数はローカルに取るべし 5 printf(" (hit return) "); // 何か表示させたほうが良いだろう 6 fflush(stdout); // just in case 7 scanf("%d", &n); // fgets(line, LINESIZE, stdin); という手も 8}
投稿2020/02/26 06:00
編集2020/03/01 22:59総合スコア1382
> 構造体の配列を初期化出来ずにクラッシュしていたようで、mallocとmemsetを使って・・・自己解決
解決したなら結構。でも「初期化出来ずにクラッシュ」というなら -O2 で実行できたことの説明がつくでしょうか？
malloc() すればスタック領域を消費しなくなるので、スタックオーバーフローだった可能性は高まったと思います。
読み直したらDreamTheaterさんもそこを疑ってますね。
詳しいことは理解できてませんが、多分使ってないOP構造体がたくさんありすぎてメモリが溢れたんだと思います。その時でも１６Gもメモリを積んでいるので暴走しないはずだと思ったんですが、難しい問題なんですね。勉強になりました。変数を減らしてみます。自分としてはOP構造体より、実装の楽な（メモリ消費の多い）vec（ベクトル型）構造体のほうが問題だと思っているので、今vec型を使わないで、全てOPだけで処理できるようにモジュールを改善しているところです。そのコードはobenor.cにあります。もしか医療に成功すれば、多項式の次数が高いときでも少ないメモリで計算することができるようになります。今は多項式の次数＝添字なので次数が高くなるとそれだけメモリの消費量も増えます。しかしOPだけにすれば後の数だけメモリを消費することになるので、そんなに大きなサイズにはならないと考えています。
できればフォークして別プロジェクトにして私のコードを活かしてもらいたいですけど、今のところ誰にも相手にされてないようですｗ
プログラムコードを改善することをリファクタリングと言います。リファクタリングを進めるためには検算できるデータが必要です。プログラムのテスト自体がそうなんですけど、入力したデータを処理したら計算結果はこうなる、という検査（ブラックボックステストと呼ぶ）ができないと”改善”した事が正しかったどうかがわからないので、怖くて直しようがありません。
今の表示は、何が行われているのか、何が入力で何が答えなのか、門外漢にはさっぱりです。そこをなんとかしたらどうでしょうか。
今の所計算結果の検証は、SageMathという数学ソフトを使っているのですが、それを使ってテストデータを作ってみます。
私のコードが役に立つ可能性は少ないのですが、OP構造体という実装のアプローチが良くない可能性はありますか？
削除
ありがとうございます。今朝暗号が完成したので、ブロックチェーンに応用しようとしています。
今の私の能力では無理かもしれませんがやってみます。
rubato様
レビューありがとうございます。このプログラムをブロックチェーンに応用したいのですが難しいでしょうか？何かご存知でしたら教えてください。
このコード品質では無謀です。
他力本願じゃだめですよね。そんなことしたら私が作った意味がなくなってしまう。だから、だめｗ。
２ヶ月もやっているので飽きちゃった感じ。プログラムの整形と並行してやってみることにします。
プログラミングを仕事にしなくてよかったです。殆どの人はしたことないでしょうけどｗ．
才能のない人が無理やりコードを書くとこうなるといういい見本ですw。
scanfを入れているのは、表示を確認するために一時停止をしたいからで、system(PAUSE)とやっても
うまく行かなかったので仕方なくやりました。どうすればいいのでしょう？
三蔵法師になった気分でがんばります。
これだけ書けるんだから基本的な力はあるんですよ。あとは、身についてないコーディング技術、適切なコーディングスタイル・設計手法を学ぶこと。
作りたいものがあるうちは才能云々は関係ないと思います。
ブロックチェーンを作りたいです。
nについては特に意識してませんでした。もしファイルを扱うことになっていたら、原因不明のバグになっていた可能性がありますね。気をつけます。
あなたの回答
tips
プレビュー
