アカウント名:
パスワード:
警告を無視しちゃいかんという教訓を得たと。
こんな感じに、if分を一行にまとめてしまえば、こんな不具合出なかった気がします。
if ((err = SSLHashSHA1.update(&hashCtx, &serverRandom)) != 0) goto fail;if ((err = SSLHashSHA1.update(&hashCtx, &signedParams)) != 0) goto fail;if ((err = SSLHashSHA1.final(&hashCtx, &hashOut)) != 0) goto fail;
一行が長くなるのを嫌ったのかな?
一行にまとめるコーディングは嫌いですし、職業プログラマーはあまり使うべきではないとも思っています。なので対処はif分の処理が1行でも必ず括弧で括る事です。
そうですね。どっちにしろ2行以上あれば必ず括弧が必要な訳だし、if文書いたら脊髄反射で{}を書いてしまうくらいでちょうどいいです。
心掛ける以前に、オートインデントのために{}は必須でしょう自分でTabキーを押すなんて面倒臭すぎて死んじゃう
括弧がないくらいでオートインデントできないエディタは使うべきじゃないんじゃないかな
スタイルの問題ではないでしょこれは。こういう重要な箇所くらいテストで100%のカバレッジを確保しろよと。どーでもいいとこに心血注ぐ必要はないけどさ。
gotoとか使うとか{}を書かないとかするなら、全パターン網羅しろって感じではあります。
この手のミスを防ぐ為にコーディング規約で縛っておくのが良いと思っています。
goto fail;くらいなら1行にした方が1行抹消でif文と処理が両方消せていいんじゃないかな。}{の置き方次第でif文1行抹消で前後がうまく繋がっちゃう可能性もあるし。
四角四面なルールに沿って行動することで、プログラマは脳みそがルーチンワークに最適化されて、脳みそを使わず脊髄で仕事をするようになる。
それが最悪の結果をもたらす。
コーダーがルーチンワークに最適化されずにどうする?勘違いしてないか。
してない。だから駄目ソフトができあがる。
unreachable codeの検出は、停止性問題と同じなんで、goto文みたいな自由度の高い構文ではうまく動かないんじゃないのかね。それ以前にgccもclangもgotoの警告はやる気ないから
XCodeとかいうゴミだとどうかわからないけど、AppCodeだと普通に警告されたよ
> unreachable codeの検出は、停止性問題と同じなんで
違いますね
到達する可能性があるか、全くないかの判定だけすればいいので
#define ErrorCheck(f) do { if ((err = f) != 0) goto fail; } while (0)
ErrorCheck(SSLHashSHA1.update(&hashCtx, &serverRandom));ErrorCheck(SSLHashSHA1.update(&hashCtx, &signedParams));ErrorCheck(SSLHashSHA1.final(&hashCtx, &hashOut));
ってやりたくなるんだけど、ダメ?
do while いるの?
マクロをdo-while(0)で囲むワケhttp://d.hatena.ne.jp/ryochack/20111022/1319252481 [hatena.ne.jp]
do whileがなかったら、if ( foo == true ) ErrorCheck()else ErrorCheck()これがif ( foo == true ) if(~) goto fail;else if(~) goto fail;になってしまう。凡ミスを防ぐための処理。fooがtrueでない場合にelseのほうを実行してほしいのに、マクロ展開すると、fooがfalseだとなにも実行されないという。まぁ、これもカッコつけろよって話なんだけど。
doとwhile(0)が必要な理由になってないんだけど・・・
マクロ関数を普通の関数のように最後に ; を付けて適切に展開されるようにするため。
do while (0) なしだと
if () ErrorCheck(...);else ErrorCheck(...);
が
if () { ... };else { ... };
となり、 if の終わりの {} の後の ; が空文となるので、 else の行で文法エラーとなる。
do while (0) ありだと
if () do { ... } while (0);else do { ... } while (0);
と、 if と else が適切に対応する形になるよう展開される。
do~while(0)を使う理由はわかったが、if、elseには必ず{~}をつける、というルールを徹底した方が良い。レビューの度にこういう議論をしたくない。
if() ErrorCheck()else ErrorCheck()
でおk
#2550910だけど、私もそう思う。if文はカッコがないとコンパイルエラーにしちゃえばいいのにねぇ。
考えてみたら、defineマクロにgoto入れられてたら、do while関係なしに投げ捨てたくなるわ。
スマホで書き込んでるから、説明簡素化しすぎて伝えられなくてごめん。詳細に書いてくれた人ありがとう。
今回のケースでは必要ないけど、#2550910がいうような凡ミスを防ぐために習慣的に付けるよね。多少のタイプ量以上のオーバーヘッドもないし。
後から読むときもループなのか、終了条件は・・・ループじゃねえのか!なんでdo while付いてんだ!ってオーバーヘッドがあるじゃん。#2550910がいうミスを防ぐなら括弧だけでいいのに
do while (0)は使い古されたテクニックだから、そういうもんだと一度覚えればいいだけだが。こういうBad Know Howを嫌うのも仕方ないけど、Cだからね。で、括弧だけではダメな理由は、
if (foo == true) ErrorCheck();else ...
がコンパイルエラーになること。やってみそ。
オーバーヘッドがあるかないかはコンパイラ依存。do~while(0)の場合に{~}と同じアセンブラを吐け、という規約はないだろ。
コードの見通しがすごく悪くない?
個人的にgotoをマクロで隠さないでほしい、セットで使うようなdefineならわかるけど。ここだと短縮されてるわけでもないし、見やすくなってるとも思えない。オプションでマクロを切り替えるとかでもなさそうだし、マクロにするのはやりすぎに感じる。
いまだにdoとwhile(0)を使う理由がわからない。{~}だけでよいのでは。
#define ErrorCheck(f) if ((err = f) != 0) goto fail でおk
それは潜在的に危険なコードです。
ErrorCheck()を使う側で、以下ようなコードを書くと、
if (.../*条件A*/) ErrorCheck(...);else .../*処理B*/;
この見た目と違って、実際のマクロ展開は(改行と字下げを付けると)、
if (.../*条件A*/) if ((err = ...) != 0) goto fail; else .../*処理B*/;
となり、「処理B」が「条件A」のelse部でなく、「(err = ...) != 0」のelse部にくっついてしまいます。
else節は最も近いif文にくっつくという構文規則になっているので、 マクロにelse節無しの剥き出しのif文を埋め込むのは危険なのです。
do〜while(0)で囲めば、このような副作用が無くなるので、より安全なマクロ定義となります。
呼び出し元のスコープに影響与えるマクロの癖に関数と同じように使おうというのが間違いかなぁと思います。やっぱりC言語とそのマクロは滅びるべき
こういう生半可を産んでしまうので、やはりマクロは鬼門ですな。constやインライン関数など、マクロを使わずにすむ仕掛けを使いたいところだけど、今回のようなものはそれらじゃ書けない。マクロが悪いのか、gotoが悪いのか...
だめ、ぜったい。これは、エラーチェックだけしてるんじゃなくて、実行してからエラーだったらgoto failしてるんだから、もっとまともな名前にすべき。
do-whileのテクニックで云々している上級者の方々もそこんとこよろしく。
単文でも必ずブロック化しろってのがマイジャスティス。そもそもifの()内で関数呼び出しして、しかも真偽判定用の式内で代入してる時点で超気持ち悪いです。
これ、条件式が追加される仕様変更が来たときに SSLHashSHA1.update関数とかが呼ばれなくなる可能性があるから書き直しになるんですが。
このコードはCではなくてC++なのでは。SSLHashSHA1.updateはメンバ関数を呼び出している様に見えるけど同じディレクトリにcppファイルもあるみたいだし。C++11以降が使える処理系なら、こんなふうにgotoを使わずにラムダ式でreturnしたほうがいいのでは。int errconst bool flag=[&](){ if((err=SSLHashSHA1.update(&hashCtx, &serverRandom))!=0){return false;} if((err=SSLHashSHA1.update(&hashCtx, &signedPa
"."が使われている場合でも、C++のメンバ関数ではなく構造体のメンバに関数ポインタがあり、それを呼び出しているだけの可能性もあるので呼び出し側だけではC++noコードだとは判断が付きません。
その構造体の定義は見つける前にGive upしましたが、関数ポインタをメンバに持つ似たような構造体定義は同じディレクトリのsslTypes.h [apple.com]の下の方にもあります。
Perl教徒「後置できるなら一行でいいけど」
gccじゃなくてclangでしょう。
変にかっこつけてそういうプログラム書く人がいるから、デバッグがめんどくさいし、デバックでバグが生まれるんだよ。
# goto 文読みにくいわ!
今回はかっこつけてないから発生したバグですよ。
if文を1行にまとめるよりも1個にまとめた方がいいんじゃないの?
if (err = (SSLHashSHA1.update(&hashCtx, &serverRandom) || SSLHashSHA1.update(&hashCtx, &signedParams) || SSLHashSHA1.final(&hashCtx, &hashOut))) goto fail;
それエラー(0以外)の時しかfinalまで呼ばれない上にerrに0か1しか入らないじゃん
途中でエラー(0以外)が出たらfinalが呼ばれないんだけど。それは間違ってない。ただ、Cの仕様では論理演算子は1か0しか返さないようだから、その点に関してはあなたの指摘の通り。
でも、gccとかだと、コンパイラが警告を出すこともなく、動作も正常だったと思うんだけどな。LISPみたいに書けて、便利だなと思った記憶が。記憶違いかな。
この場合は普通に&&でつなげばいいと思うのだが…そういう話ではない?
より多くのコメントがこの議論にあるかもしれませんが、JavaScriptが有効ではない環境を使用している場合、クラシックなコメントシステム(D1)に設定を変更する必要があります。
コンピュータは旧約聖書の神に似ている、規則は多く、慈悲は無い -- Joseph Campbell
unreachable codeの警告は無視されたのか (スコア:0)
警告を無視しちゃいかんという教訓を得たと。
こんな感じに、if分を一行にまとめてしまえば、こんな不具合出なかった気がします。
if ((err = SSLHashSHA1.update(&hashCtx, &serverRandom)) != 0) goto fail;
if ((err = SSLHashSHA1.update(&hashCtx, &signedParams)) != 0) goto fail;
if ((err = SSLHashSHA1.final(&hashCtx, &hashOut)) != 0) goto fail;
一行が長くなるのを嫌ったのかな?
Re:unreachable codeの警告は無視されたのか (スコア:3, すばらしい洞察)
一行にまとめるコーディングは嫌いですし、職業プログラマーはあまり使うべきではないとも思っています。
なので対処はif分の処理が1行でも必ず括弧で括る事です。
Re:unreachable codeの警告は無視されたのか (スコア:1)
そうですね。
どっちにしろ2行以上あれば必ず括弧が必要な訳だし、if文書いたら脊髄反射で{}を書いてしまうくらいでちょうどいいです。
Re: (スコア:0)
心掛ける以前に、オートインデントのために{}は必須でしょう
自分でTabキーを押すなんて面倒臭すぎて死んじゃう
Re: (スコア:0)
括弧がないくらいでオートインデントできないエディタは
使うべきじゃないんじゃないかな
Re:unreachable codeの警告は無視されたのか (スコア:1)
スタイルの問題ではないでしょこれは。
こういう重要な箇所くらいテストで100%のカバレッジを確保しろよと。
どーでもいいとこに心血注ぐ必要はないけどさ。
Re: (スコア:0)
gotoとか使うとか{}を書かないとかするなら、全パターン網羅しろって感じではあります。
Re: (スコア:0)
この手のミスを防ぐ為にコーディング規約で縛っておくのが良いと思っています。
Re: (スコア:0)
goto fail;くらいなら1行にした方が1行抹消でif文と処理が両方消せていいんじゃないかな。
}{の置き方次第でif文1行抹消で前後がうまく繋がっちゃう可能性もあるし。
Re: (スコア:0)
四角四面なルールに沿って行動することで、
プログラマは脳みそがルーチンワークに最適化されて、
脳みそを使わず脊髄で仕事をするようになる。
それが最悪の結果をもたらす。
Re: (スコア:0)
コーダーがルーチンワークに最適化されずにどうする?
勘違いしてないか。
Re: (スコア:0)
してない。
だから駄目ソフトができあがる。
Re:unreachable codeの警告は無視されたのか (スコア:1)
unreachable codeの検出は、停止性問題と同じなんで、goto文みたいな自由度の高い構文ではうまく動かないんじゃないのかね。
それ以前にgccもclangもgotoの警告はやる気ないから
Re: (スコア:0)
XCodeとかいうゴミだとどうかわからないけど、AppCodeだと普通に警告されたよ
Re: (スコア:0)
> unreachable codeの検出は、停止性問題と同じなんで
違いますね
到達する可能性があるか、全くないかの判定だけすればいいので
Re: (スコア:0)
#define ErrorCheck(f) do { if ((err = f) != 0) goto fail; } while (0)
ErrorCheck(SSLHashSHA1.update(&hashCtx, &serverRandom));
ErrorCheck(SSLHashSHA1.update(&hashCtx, &signedParams));
ErrorCheck(SSLHashSHA1.final(&hashCtx, &hashOut));
ってやりたくなるんだけど、ダメ?
Re: (スコア:0)
do while いるの?
Re:unreachable codeの警告は無視されたのか (スコア:3)
マクロをdo-while(0)で囲むワケ
http://d.hatena.ne.jp/ryochack/20111022/1319252481 [hatena.ne.jp]
Re: (スコア:0)
do whileがなかったら、
if ( foo == true )
ErrorCheck()
else
ErrorCheck()
これが
if ( foo == true )
if(~) goto fail;
else
if(~) goto fail;
になってしまう。凡ミスを防ぐための処理。
fooがtrueでない場合にelseのほうを実行してほしいのに、マクロ展開すると、fooがfalseだとなにも実行されないという。
まぁ、これもカッコつけろよって話なんだけど。
Re: (スコア:0)
doとwhile(0)が必要な理由になってないんだけど・・・
Re:unreachable codeの警告は無視されたのか (スコア:1)
マクロ関数を普通の関数のように最後に ; を付けて適切に展開されるようにするため。
do while (0) なしだと
if ()
ErrorCheck(...);
else
ErrorCheck(...);
が
if ()
{ ... };
else
{ ... };
となり、 if の終わりの {} の後の ; が空文となるので、 else の行で文法エラーとなる。
do while (0) ありだと
if ()
ErrorCheck(...);
else
ErrorCheck(...);
が
if ()
do { ... } while (0);
else
do { ... } while (0);
と、 if と else が適切に対応する形になるよう展開される。
Re: (スコア:0)
do~while(0)を使う理由はわかったが、
if、elseには必ず{~}をつける、というルールを徹底した方が良い。
レビューの度にこういう議論をしたくない。
Re: (スコア:0)
if()
ErrorCheck()
else
ErrorCheck()
でおk
Re: (スコア:0)
#2550910だけど、私もそう思う。
if文はカッコがないとコンパイルエラーにしちゃえばいいのにねぇ。
考えてみたら、defineマクロにgoto入れられてたら、do while関係なしに投げ捨てたくなるわ。
スマホで書き込んでるから、説明簡素化しすぎて伝えられなくてごめん。
詳細に書いてくれた人ありがとう。
Re: (スコア:0)
今回のケースでは必要ないけど、#2550910がいうような凡ミスを防ぐために習慣的に付けるよね。多少のタイプ量以上のオーバーヘッドもないし。
Re: (スコア:0)
後から読むときもループなのか、終了条件は・・・ループじゃねえのか!なんでdo while付いてんだ!ってオーバーヘッドがあるじゃん。
#2550910がいうミスを防ぐなら括弧だけでいいのに
Re: (スコア:0)
do while (0)は使い古されたテクニックだから、そういうもんだと一度覚えればいいだけだが。
こういうBad Know Howを嫌うのも仕方ないけど、Cだからね。
で、括弧だけではダメな理由は、
if (foo == true)
ErrorCheck();
else
...
がコンパイルエラーになること。やってみそ。
Re: (スコア:0)
Re: (スコア:0)
オーバーヘッドがあるかないかはコンパイラ依存。
do~while(0)の場合に{~}と同じアセンブラを吐け、という規約はないだろ。
Re: (スコア:0)
コードの見通しがすごく悪くない?
Re: (スコア:0)
個人的にgotoをマクロで隠さないでほしい、セットで使うようなdefineならわかるけど。
ここだと短縮されてるわけでもないし、見やすくなってるとも思えない。
オプションでマクロを切り替えるとかでもなさそうだし、マクロにするのはやりすぎに感じる。
Re: (スコア:0)
いまだにdoとwhile(0)を使う理由がわからない。
{~}だけでよいのでは。
Re: (スコア:0)
括弧だけだとif文に後続する複文になってしまうから、do while(0)で囲んで単文にすることで、括弧なしのif文で使ったとき正しく展開されるようになる。
ただしif文そのものは単文だから、今回のif ((err = f) != 0) goto failはdo while(0)で囲む必要は全くない。
#define ErrorCheck(f) if ((err = f) != 0) goto fail
でおk
# 末尾に;つけないでおくのがポイント
Re:unreachable codeの警告は無視されたのか (スコア:3, 参考になる)
それは潜在的に危険なコードです。
ErrorCheck()を使う側で、以下ようなコードを書くと、
この見た目と違って、実際のマクロ展開は(改行と字下げを付けると)、
となり、「処理B」が「条件A」のelse部でなく、「(err = ...) != 0」のelse部にくっついてしまいます。
else節は最も近いif文にくっつくという構文規則になっているので、 マクロにelse節無しの剥き出しのif文を埋め込むのは危険なのです。
do〜while(0)で囲めば、このような副作用が無くなるので、より安全なマクロ定義となります。
Re: (スコア:0)
呼び出し元のスコープに影響与えるマクロの癖に関数と同じように使おうというのが間違いかなぁと思います。
やっぱりC言語とそのマクロは滅びるべき
Re: (スコア:0)
こういう生半可を産んでしまうので、やはりマクロは鬼門ですな。
constやインライン関数など、マクロを使わずにすむ仕掛けを使いたいところだけど、今回のようなものはそれらじゃ書けない。
マクロが悪いのか、gotoが悪いのか...
Re: (スコア:0)
だめ、ぜったい。
これは、エラーチェックだけしてるんじゃなくて、
実行してからエラーだったらgoto failしてるんだから、
もっとまともな名前にすべき。
do-whileのテクニックで云々している上級者の方々もそこんとこよろしく。
Re: (スコア:0)
単文でも必ずブロック化しろってのがマイジャスティス。
そもそもifの()内で関数呼び出しして、しかも真偽判定用の式内で代入してる時点で超気持ち悪いです。
これ、条件式が追加される仕様変更が来たときに SSLHashSHA1.update関数とかが呼ばれなくなる可能性があるから書き直しになるんですが。
そもそも (スコア:0)
このコードはCではなくてC++なのでは。SSLHashSHA1.updateはメンバ関数を呼び出している様に見えるけど
同じディレクトリにcppファイルもあるみたいだし。
C++11以降が使える処理系なら、こんなふうにgotoを使わずにラムダ式でreturnしたほうがいいのでは。
int err
const bool flag=[&](){
if((err=SSLHashSHA1.update(&hashCtx, &serverRandom))!=0){return false;}
if((err=SSLHashSHA1.update(&hashCtx, &signedPa
関数ポインタ (スコア:1)
"."が使われている場合でも、C++のメンバ関数ではなく
構造体のメンバに関数ポインタがあり、それを呼び出しているだけの可能性もあるので
呼び出し側だけではC++noコードだとは判断が付きません。
その構造体の定義は見つける前にGive upしましたが、関数ポインタをメンバに持つ似たような構造体定義は同じディレクトリの
sslTypes.h [apple.com]の下の方にもあります。
Re: (スコア:0)
Perl教徒「後置できるなら一行でいいけど」
Re: (スコア:0)
リンク先にもあるけど、XCode (から呼ばれるgcc)はこのケースで警告出さないらしいよ。
Re: (スコア:0)
gccじゃなくてclangでしょう。
Re: (スコア:0)
Re: (スコア:0)
変にかっこつけてそういうプログラム書く人がいるから、デバッグがめんどくさいし、デバックでバグが生まれるんだよ。
# goto 文読みにくいわ!
Re:unreachable codeの警告は無視されたのか (スコア:4, おもしろおかしい)
今回はかっこつけてないから発生したバグですよ。
Re: (スコア:0)
if文を1行にまとめるよりも1個にまとめた方がいいんじゃないの?
Re: (スコア:0)
それエラー(0以外)の時しかfinalまで呼ばれない上にerrに0か1しか入らないじゃん
Re: (スコア:0)
途中でエラー(0以外)が出たらfinalが呼ばれないんだけど。それは間違ってない。ただ、Cの仕様では論理演算子は1か0しか返さないようだから、その点に関してはあなたの指摘の通り。
でも、gccとかだと、コンパイラが警告を出すこともなく、動作も正常だったと思うんだけどな。LISPみたいに書けて、便利だなと思った記憶が。記憶違いかな。
Re: (スコア:0)
この場合は普通に&&でつなげばいいと思うのだが…そういう話ではない?