☕ コーディングスタイルで盛り上がった時のコーヒーブランチ ☕

[KENCHjp]

一番コミットからは遠い私がIssue起こすのもはばかったのですが、ひとまず提案。

このチームは固有の「コーディング規約」は規定していない、しないでいいのかなと思っています。
第三者に納品するわけでもなく、参画する人のスキル差は多少あってしかるべきですが、
参画しようという人はそれなりな知識がある人かと思うので、
それよりも開発スピードだったり効率だったりを考えると
ガチガチの「コーディング規約」は足枷にしかならないと思っています。

ですが、スキル差というよりは「ねんき」の差で、
PRの中にコーディングスタイルはこうしたほうがよいああしたほうがよい、
と本来解決せねばならない課題とは別にコーディングスタイルで
PRの議論の半分以上を占めるとそれはそれで本末転倒な気もします。

改めて書くまでもありませんが、PRの中では、

• 解決せねばならない課題が解決されているか（不具合の解消や追加機能の検証）
• ゴミがないか（不要なコメントや未使用な変数やデバッグコードなどの処理の残骸等）
• 最低限の読みやすさ（インデントがなされているか、変数名が誤解しにくい等）

に集中して、「分かりやすさ、再利用のしやすさ、不具合の作りこみにくさ」などで培った経験からくる差で
議論が盛り上がってきたら、いったん冷静になったほうがいいかなと、
張本人な私が言うのもなんですが後から見て思いました。

そうなったら、またはなりかけたら合言葉的なものがあったら、
「あ、そうなのかもしれない、いったん冷静になろう」って思えるかなと。

[KENCHjp]

事例１

if( bDelete && m_pShareData->m_sNodes.m_nEditArrNum == 0 ){

の
m_pShareData->m_sNodes.m_nEditArrNum == 0
の部分が
m_pShareData->m_sNodes.m_nEditArrNum
こう修正されていました。
私も「m_pShareData->m_sNodes.m_nEditArrNum == 0」のほうが理解しやすいと思いましたが、
修正者の意図は、比較演算子は何かと比較するものであって、ゼロかゼロじゃないかを判断したいわけではなく、
本来であれば、editors.empty()としたかった、比較したい訳ではないのと、また==0が冗長ではないかとの判断からかと思います。
それはそれで理解できます。
が、数値をそのままbool判断に使うのはその数値が0かそうじゃないかにかかわらず、
非常に気持ち悪いのは、Cがまだboolの概念がなかったからか、他の言語が数値に対するbool判断がまちまちで、
明示的に書いておいたほうが脳内変換しなくて済むからかと思いました。(まだ変数がbool型なら理解できるのですが個人的にはbool型でも==Trueとか書くタイプ)

if( bDelete && m_pShareData->m_sNodes.m_nEditArrNum ){

こう書かれていても、頭の中では一旦考えて、

if( bDelete && m_pShareData->m_sNodes.m_nEditArrNum == 0 ){

こう変換している自分が居ます。
もしくは本当にそうでいいんだっけ？&&と==はどっち優先だっけ？って考えている自分がいます。

例えば冗長という観点で言うと、

if ( $aaa === 1 ) {
  doAaa();
}

これは、

if ( $aaa === 1 )
  doAaa();

こう書けますが冗長だからと言って下で書く人のほうが少ないのはいうまでもないかと思いますが、
会社のコーディング規約では、

if ( $aaa === 1 ) {
  doAaa();
} else {
  //
}

こうしろなんてところもあります（言語によってコメント行だけだとエラーになるのでなにがしかnopな処理を書いたほうがいいかも）

例えば、

m_pShareData->m_sNodes.m_nEditArrNum == 0

これは、

isEmpty(m_pShareData->m_sNodes.m_nEditArrNum)

にして、isEmptyはオーバーロード関数にして、引数がintの時は0かそれ以外かでTrue/Falseを返すとか、
そういう関数があったほうが両者見やすくていいのかもしれません。

で、結論ですが、私は「どっちでもいいんじゃないか」と改めて思いました。

m_pShareData->m_sNodes.m_nEditArrNum == 0

↑これをもう読みにくいと思うのは私から見たらもう「ニュータイプ」なんじゃないかと。
結局どっちの書き方でも不具合ではないので、そのコーディングをした人のやりやすいようにやるのが吉なのかなと。
すいません、オチが「どっちでもいい」で。

[kobake]

Issue 立てありがとうございます！GJ です！

[kobake]

PR 趣旨から外れた議論が勃発したら、ひとまずはここを避難地にしましょうかね ：）

[KENCHjp]

@kobake さん
「避暑地」
いいすね。
白熱しだしたら「避暑地に行きましょう」ですね。

[kobake]

ヘッダの中で using

sakura-editor/sakura#200 で上がっていた話題ですが、ヘッダ内での using については個人的には断固拒否なスタンスです。
もちろん例外として「仕方ないパターン」がある可能性はありますが、そうでもなければヘッダ内で using はしてほしくないです。せっかく namespace 分けてる意味がなくなっちゃいます。こういうことをする合理的な理由があるのであれば逆に知りたいところです。

個人的にはコーディングスタイルは幅広く許容していきたい（と思っている）スタンスですが、ここについては譲れないところであります。ヘッダ内 using を許容するスタンスの人って他で見たことないです。

※個人プロダクトでは楽するためにやっちゃうことは稀にありますけど。本来は絶対にやりたくない作法です。

[kobake]

同じく sakura-editor/sakura#200 でのネタ。

SetPriorityClass の結果を assert するのは間違っています。
assert は論理的に常に成立する条件を保証するためのものだからです。

これに同意です。
既に該当 PR 内では assert は削除されたらしいので避暑地側に書きます。

自分は assert に入れる条件は、原則「静的な論理値」と考えています。
SetPriorityClass の結果は API 呼び出しの動的な結果であり、明らかに静的ではなく値の予測が付かないものです。

そういう箇所では assert ではなく条件分岐でログ出したり例外出したりその他エラー対策処理を入れるべきところであったかと。

[KENCHjp]

C++よくわかってないんで、assertとか、ヘッダの中で usingとかの弊害が身に染みてないですが、「楽できる何か」があるんでしょうねぇ・・・

ただRPの中にあった、

(追記) ヘッダがヘッダをインクルードするのがそもそもの間違いとか、そういうことなんでしょうか。

ヘッダがヘッダをインクルードするのは間違いじゃないと思うんですけども、どうなんでしょ。
ブラックボックスの機能の中で呼び出している第三の機能のために、呼び出し元から、その途中のソース含めヘッダーを追記しなければならないのはなんか違うきがしますけどね。。。

内容が高度すぎてついていけてませんが。

[kobake]

ヘッダがヘッダをインクルードするのは間違いじゃないと思うんですけども、どうなんでしょ。

あっちでは既にコメント入っていたので追加コメントは入れませんでしたが、ヘッダがヘッダをインクルードするのは普通。というかむしろ依存関係の意識をヘッダ利用者に負担させない意味で推奨です。

たとえば a.h が b.h に依存していて b.h が c.h に依存しているとき、ヘッダ内ヘッダインクルードの策をとっていない場合、
a.h を利用したい .cpp 側では a.h の前に b.h をインクルードする必要があり b.h をインクルードする前に c.h をインクルードする必要があり……、などという利用制約があっては、ヘッダ利用者としてはたまったものではありませんし、インクルード文のコード量が増えて管理が煩雑になります。依存関係が変わったときのコード変更量もかなり多くなりフットワーク激重になります。

ちなみに windows.h 自体にもインクルードガード付いてるので、まかり間違って windows.h を2回以上 include してしまっても問題ないです。

[kobake]

assertとか、ヘッダの中で usingとか

これについては自分の場合は身になじみすぎて言葉足らずになってしまうことがあり反省。
ただ、今これの話をするとヘッダの話と assert の話と using の話が混ざりそうなので、これについての深い話をするタイミングはちょっと待ちますｗ

[ds14050]

ヘッダがヘッダをインクルードするのは間違いじゃないと思うんですけども、どうなんでしょ。

まったくその通りだと思うんですけども

#pragma onceもインクルードガードも、本来使うべきものじゃないっす・・・

というのを受けて、型の再定義によるコンパイルエラーを避ける方法があるだろうかと考えてみた結果がそうなりました。あくまでも仮定ありきです。

普通に考えてインクルードガードがない世界では、#include ＜string＞ する a.h と、#include ＜string＞ する b.h の、両方をインクルードする main.cpp において、std::string の定義がダブってしまって、それがテンプレートならテキスト比較の結果によってお目こぼしされたりするんですが(嘘でした)、コンパイルエラーになるんですね。

これをインクルードガードなしで解決するには、main.cpp に #include ＜string＞ と #include "a.h" と #include "b.h" を並べ、a.h と b.h から #include ＜string＞ を取り除くしかないのではないかと。

google のスタイルガイドに「自己完結型ヘッダー」という言葉がありましたが、こちらはさながら「自己完結型翻訳単位」に当たります。直接・間接を問わずすべての依存ヘッダを一列に並べて把握・管理しようとする姿勢はストイックで嫌いではありませんが、主流派にはなれないでしょう。というか、こんな派閥の存在を知りません。

[ds14050]

返事 sakura-editor/sakura#200 (comment) をいただけたので、ちょっとここで続けます。

前方宣言を使い、ポインタ型や参照型を関数プロトタイプやメンバ変数の型に使うだけなら型の完全な定義をインクルードする必要がないということでした。それはインクルードガードなしで自己完結型のヘッダを目指す試みですが、それだけでは以下の制限が考えられ、それをスタイルとして集団で許容することができるかどうか。

• 継承ができない。
• インライン関数が書けない。
• メンバ変数として、ポインタや参照ではなく大きさを持ったオブジェクトを埋め込めない。

「(インクルードガードなしでの)自己完結型ヘッダー」を諦めて「自己完結型翻訳単位」を目指すならこれらの制限は乗り越えられますが、茨の道であるのは変わりません。

[berryzplus]

なにやら盛り上がっていたのに今更気付いたなり・・・。

@kobakeさん

自分は assert に入れる条件は、原則「静的な論理値」と考えています。
SetPriorityClass の結果は API 呼び出しの動的な結果であり、明らかに静的ではなく値の予測が付かないものです。

そういう箇所では assert ではなく条件分岐でログ出したり例外出したりその他エラー対策処理を入れるべきところであったかと。

完全に同意です。例外投げるコードを書きたかったけどまだ例外の投げ方に関して話できてないので、暫定策としてassertにしてました。assertにしとけば、いつか誰かがおかしい！って言うだろうという確信犯でした。一目で見破るとはさすがです・・・。

[berryzplus]

インクルードガードの話。

正直、いま現在のサクラエディタのインクルードガードはあまり好きじゃありません。
でもインクルードガード派の意見にちょっと揺れてる感じです。

こういうインクルードガードは馴染みがあります。
https://github.com/sakura-editor/sakura/blob/f2c64ee5c3b8e8f8dc7d7db33b01ee391f84cec2/sakura_core/_main/CProcessFactory.h#L15-L16

こういうインクルードガードは良くない気がしています。
https://github.com/sakura-editor/sakura/blob/f2c64ee5c3b8e8f8dc7d7db33b01ee391f84cec2/sakura_core/_os/CClipboard.h#L24-L25

後半部分が意味不明なのがよくないと思う理由です。
たぶんGUIDを埋め込んであるだけなので意味はないんだと思います。

「インクルードガードとは、意味不明な文字列をソースコードに混入させることである」というのが、
あながち間違いでない状態になってる気がします。それは嫌。

他のプロジェクトやライブラリを気軽に参照できるようにするために、
プロジェクト名を含めた絶対パスを識別IDに埋め込むようにするよ、
と言われると、「あ、それいいかも」と思ってしまうのですが。

[kobake]

僕は圧倒的に #pragma once 派です。人間が見て分かりやすく、コード量も少なく、define 定数名被りも起こりえないので。

ただ、提供するものが SDK であれば #pragma once は使うべきではないですね。できるだけ広い範囲のコンパイラで使われることを想定すべきなので。

プロダクト単位ではチームメンバーでの合意がとれていれば #pragma once 使いたいです。
もちろん状況により弊害があることも存じていますが、それは頻繁に起こることではないので問題が起こったときだけピンポイントで対処すれば良いというスタンスです。

ちなみに #pragma once は define 方式のインクルードガードに比べて高速です。
define 方式とは違い、二度目以降のヘッダ読み込み (IO) 自体が省略されるので。

[ds14050]

参考リンク>インクルードガード | 闇夜のC++

すべてのトピックを網羅する勢いです。これ以外のトピックがあれば知りたい。

[berryzplus]

@KENCHjp さん

if( bDelete && m_pShareData->m_sNodes.m_nEditArrNum ){

こう書かれていても、頭の中では一旦考えて、

頭の中で文意を変換するのは誰でもすることだと思います。
この話は「if文とは何ぞや」の理解が違うってことなんだと思ってます。

ぼくはBASICでif文を覚えました。
If :EXPR Then :STATEMENT
「もし条件式:EXPRが真なら、:STATEMENTを実行する」

:EXPRが変数なら値があるかどうかの判定です。
「変数に値がある」は「0以外の値がセットされている」です。
この脳内変換にストレスを感じたことはほぼありません。

ニュータイプなんですかね(^^;

[ds14050]

0 はれっきとした値ですよ。Ruby なら真に評価される値ですよ。

[ds14050]

話は戻りますが、サクラエディタの定められた形式はこれです。

#ifndef SAKURA_CDATAPROFILE_2DFA1040_7EC7_4593_983E_093E988F7DCFR_H_
#define SAKURA_CDATAPROFILE_2DFA1040_7EC7_4593_983E_093E988F7DCFR_H_

#endif /* SAKURA_CDATAPROFILE_2DFA1040_7EC7_4593_983E_093E988F7DCFR_H_ */

埋め込むためのマクロが共有されていたはずですが、guidgen.exe のような外部コマンドが必要だったことと、使う機会がそうそうないために、保存はしていませんでした。

[KENCHjp]

この脳内変換にストレスを感じたことはほぼありません。
ニュータイプなんですかね(^^;

たぶん、ここなんでしょうね。私はストレスだらけなんすよね。
僕も、IFを最初に覚えたのはBASICかなぁ。それでも省略したことはほぼないというか昔のBASIC(N88BASIC)も省略できなかったような(知らなかっただけかも)
でも @berryzplus さんは省略してる感覚もないってことなんすよね。
私にはニュータイプっす(笑)

[berryzplus]

0 はれっきとした値ですよ。Ruby なら真に評価される値ですよ。

C/C++では整数型変数の値は、CPUのレジスタに格納されます。
レジスタへの格納は上書きコピーでされるので
「値がある」とは「1つでもビットが立った状態」を意味します。

C/C++では整数型変数の値がレジスタに直結するので、
より高次な高級言語であるRubyと考え方が違うのは仕方ないと思います。
Rubyはあまり使ったことがありませんが、
javascriptと同じく未定義(undefined)の概念がある言語と認識しています。

javascriptの場合はこんな感じになります。

var a = 0;
if (!a) {
    if (a === 0) {
        alert('a is zero.'); //a=0はここに来る
    } else if (a === '') {
        alert('a is blank.'); //a=''はここに来る
    } else if (a === null) {
        alert('a is null.'); //a=nullはここに来る
    } else if (a !== undefined) {
        alert('a is NaN.'); //a=NaNはここに来る
    } else {
        alert('a is undefined.'); //aが未定義はここに来る
    }
} else { //else if (a) {
    alert('a has value.'); //aに何らかの値が定義されていたらここに来る
}

Rubyの実装だと if (a !== undefined) を最優先で判定する感じになるのかな？
プログラム言語は、叶えたい目的に特化するように最適化されるものだと思っています。
コンパイルを必要としない言語なので、定義／未定義の判断を優先しているのかな？

a = 0のときに if (!a) が成立しない状況に出会ったとしたら、
ぼくならきっと、インタープリタのバグを疑うと思います。
時間があったら本当のところどうなのかについて、ちゃんと勉強しなきゃな、と思ってます・・・。

[berryzplus]

う～む、Rubyでは「if :EXPRの:EXPRが0のときにも条件が成立する」ということがやはり読み取れない。

→たぶん公式マニュアル

[berryzplus]

省略してる感覚もないってことなんすよね。
私にはニュータイプっす(笑)

たぶん話まざってますね。

• if (n)を```if (n != 0)‘‘‘に脳内変換する話
• if (n)を```if ((n != 0) == true)‘‘‘に脳内変換する話

ぼくの脳内変換は前者です。
初めから変換済みの式で書いたほうが分かりやすい（＝脳内変換するのがめんどくさいｗ）については、同意できないこともないです。ケースバイケースですが、脳内変換済みの式を書いたほうが意図が伝わりやすいこともあると思っています。

@KENCHjp さんにしてた指摘は、後者の脳内変換は不要ですよね？ということです。
ぶっちゃけ、そろそろ慣れてきて「そういう考え方もある」って思えてたりしませんか？

[KENCHjp]

たぶん話まざってますね。

そうかもです。

ぼくの脳内変換は前者です。

私も前者です。
後者は、先の式の時点でbool値になっているのでそれをさらに == Trueって考え方にはならないっす。

ぶっちゃけ、そろそろ慣れてきて「そういう考え方もある」って思えてたりしませんか？

そういう考え方もあるは、事例１で書いた通りです。
でも慣れないです、なにせ、nがBool変数であってもn==Trueとか書かないと気が済まないたちなのはご存知の通りです(笑)
なので違和感ない人を「ニュータイプ」と呼んでしまいました。もしお気に障ったらすいません。

繰り返しになりますが向こうにも張ったリンクを再掲します。

各言語におけるtrue/falseまとめ
http://blog.mirakui.com/entry/20090604/truefalse

まとめ
まあこんな感じで、言語によって真偽値の事情は異なるので、 if 文では比較演算子などを省略しない方が安心ですね!!

Rubyで0(数値)が真判定されると記載あります。
私が触る言語が、C、C++、Java、JS、Perl、とDelphi（あとBashとかBATとかps1とか)なので、各々で比較演算子省略どうだっけって覚えられない年寄り脳みそがここにおりますT_T

[kobake]

この件については、機械語の話（レジスタ含め）は混ぜないほうが良いと思いますよ。
人間にとって読みやすいかどうかの話をしているので。

ゼロを偽、非ゼロを真と脳内変換できる能力はこの話に参加している方々は全員保持していると思っています。

しかしゼロ／非ゼロによる判定はどちらかというとC言語的な世界観であり、それはつまり機械語的な世界観であり、「そういう考え方もある」ことは当然知っているけれども人間的に（というか論理値であることを明示するために）そういう書き方は避けたい人が一定数いる、という文脈として自分は解釈しています。

どの PR だか忘れましたけど、変数 n を2倍にするときに n <<= 1; とするのは人間的ではないので n *= 2; としましょう、という指摘を以前にしました。これも機械側ではなく人間側に寄り添いたい理由で指摘を行いました。昔はコンパイラ事情で前者の書き方をしたほうが好ましい時代もありましたが、今はそうではないですね。bool の話もそれに近い話だと思っています。

[berryzplus]

@KENCHjp さん

後者は、先の式の時点でbool値になっているのでそれをさらに == Trueって考え方にはならないっす。

ここが合意とれればぼくは満足です。

[kobake]

補足しておくと「== True みたいな書き方や考え方を許容しない」のは確実に満場一致で合意がとれるはずです。（みなさんのスキルを汲み取った感じ）

[berryzplus]

完全に別件です。
最近の指摘で納得いってないものがあります。

処理内容に問題はありませんが、GetProfileName() が wchar_t ポインタ型なので、
正確には '\0' ではなく L'\0' との判定であるべきところです。

ぼく日本人なので、文字リテラルがchar型であるとは限らないと思ってます。
あるべき姿として、ワイド文字リテラルもwchar型であるとは限らないと思ってます。
確かに、vc++のマニュアルを見ると’a'はchar型の定数である、と書かれています。

auto ch = 'あ'; //これは何型？

「ひらがな」は2バイト文字です。char型では足りません。

ワイドでない文字定数の値は、コンパイル環境のコードページに依存します。
cp932(=日本語)なvc++でコンパイルするとchはint型になり、値は0x82a0になります。

Unicodeの場合、ちょっと状況が違いそうです。

auto ch = L'\U0002000B'; //これは何型？

\u2000B

このコードは警告C4066を吐いて意図通りにビルドされません。
\uXXXXXXXX という書き方はUnicodeコードポイントをソースコードに埋め込む書き方です。
\u10000を超えるコードポイントはUTF16ではサロゲートペアになります。
ワイド文字列リテラルの中であれば \U0002000B は 0xDC40, 0xDC0B の2文字になります。
実行すると ch は wchar_t 型になり、値は 0xDC40 になります。
マルチバイト文字と同じ感じで扱えるとラクなんですけど、そうはならないみたいです。

	auto jo = L'\U0002000B';
	auto jostr = L"八\U0002000B島";
	auto ret = wcschr(jostr, jo);

wcschrの第二引数の型はwchar_tです。
strchrの第二引数に合わせてintにしたらいいのに、何故かwchar_tです。
上記でjoには上位サロゲートの値が入っているのですが、wcschrではそれっぽい値が返ります。

不思議！

	auto jo2 = L"\U0002000B";
	auto jostr = L"八\U0002000B島";
	auto ret = wcsstr(jostr, jo2);

現状のvc++ではこうするのがベターなのかな、と思っています。
ちなみに、「八丈島」の「丈」の字を本来どう書くかについて、ぼくはよく知りません。

本題に戻します。

正確には '\0' ではなく L'\0' との判定であるべきところです。

コードポイントが0～127の文字をASCII文字と呼ぶのはご存じだと思います。
この128文字については、cp932前提ならUnicodeに完全互換です。
文字値と文字リテラルの比較が「文字コードの数値比較」であることを考えると、
（ASCII文字に関しては）どちらで書いても構わないということになると思います。

どちらで書いても構わないならタイプ数の少ないほうがよいです。
ascii文字かどうか判断するのがめんどい、というなら分からなくもないんですが。

[kobake]

コーディングスタイルの Issue 分けていきますかね。

幸い management-forum の他の話題が活発なわけでもないのでコーディングスタイル Issue が乱立しても管理話題が埋もれるとかの問題は当面起こらなそう。

[kobake]

たしかに 'あ' は int になりますね

[KENCHjp]

ぼくはアイスコーヒー派です 😄

どっちでも行けますが、ガムシロは入れない派です。

[ds14050]

セルフつっこみ。

サクラエディタの定められた形式はこれです。

というのは正確ではありませんでした。起源はこんな感じで、使う人がいたということです。guidgen.exe もいりませんでしたね。

S:69,25,07/11/28,19:49,,
T:RE: インクルードガード
N:げんた
M:
手で入力すると間違える，他のファイルと衝突するという問題を解決するために，ガード挿入マクロをつくりました．
http://sakura.qp.land.to/?Macro%2F%C5%EA%B9%C6%2F184
アンダーバーで始まるマクロは予約済みのようなので，SAKURA_ファイル名_GUID_H_ のようなガードを生成するようにしてみました．二回実行すると二つ入ってしまう点はご容赦ください．

#pragma onceについて，gccのマニュアルではobsolete featureになっているので，世の中の流れとしては縮小傾向なのかなと．
http://gcc.gnu.org/onlinedocs/gcc-4.0.3/cpp/Obsolete-once_002donly-headers.html
ファイルのパスだけで同一性が判断されるのでsymbolic linkを使った場合に問題が出るみたいです．(ここでは関係ないですが)
.

[berryzplus]

冒頭を思いっきり「昭和６９年」と読みました。
「平成」はいったい何年まで続くのやら・・・。

[kobake]

auto の利用ポリシーについてはなんとなく意識合わせておいたほうが良いと思いました。
なんでもかんでも auto にしちゃうと、せっかく型が人の目で見やすく明示されている状態をなくしてしまうので嫌う人は嫌います。僕も状況に応じてですが、auto 嫌がることは割とあります。

なお昔の C++ であった「iterator みたいな長い型を手書きする必要があった」文化については徹底的にFXXK ですので、そのあたりを auto で綺麗にするのは大賛成です。

なのでこういうのは歓迎。
sakura-editor/sakura#288

[beru]

今はガチガチなコーディング規約が有るわけではなさそうで場所によって記述スタイルがまちまちですね。個人的にはヨーダ記法もシステムハンガリアンもかなり好きではないですが信奉者達からするとこういうのは冒涜的な発言で粛清の対象でしょうか…。

.clang-format ファイル用意して統一するようにするのは有りだと思いますが、appveyor とかでチェックするのはどうかなと思います。厳しくした後にプロジェクトが廃れちゃうと（運用ルールを厳しくしたのが原因かはともかく）元も子もないです。

[KENCHjp]

粛清の対象でしょうか…。

ここは多様性を重んじるチームだと信じています。
ただ自分の信念を押し殺す必要もないのかなと。

ですが、敵はバグや実装機能であることをふと一息ついて慮る。。。

[beru]

自分は assert に入れる条件は、原則「静的な論理値」と考えています。
SetPriorityClass の結果は API 呼び出しの動的な結果であり、明らかに静的ではなく値の予測が付かないものです。

静的な というのが定数式と同一かどうかは分からないですが、コンパイル時に判定出来るものであれば static_assert を出来る限り使うのが良いと思います。

そういう箇所では assert ではなく条件分岐でログ出したり例外出したりその他エラー対策処理を入れるべきところであったかと。

条件判定してエラー対策を入れるべき箇所では入れるのが正解なので、そういう個所を assert で済ませてしまうのは手抜きですが、戻り値を全くチェックしないよりかはマシなのかもしれません。

assert が活用出来るのは本来呼び出し側できちんとした値を引数に入れる事が前提だけれどもデバッグ(#ifndef NDEBUG)では念のためチェックしたい、という場合でしょうか。

あとはエラー処理を律儀に書くのが大変なのでとりあえずは assert を埋め込んでチェックしておいて、エラー処理の記述は後回しにするときとか。。まぁ手抜きと見分けるのが難しいですね。

[kobake]

静的なというのはちょっと正確でなかったですね。仕様や前提を宣言する的な用途で使う感覚かなーと。

以下のような assert は割と許容しちゃいますね。

void f(void* p){
  assert(p);
  ....
  ....
}

さらに踏み込むと、論理値といいながらここでは生ポインタ突っ込んじゃってますが、こういうのはあえてサボっちゃうことも多いです（サボっているという意識はある）。

[kobake]

.clang-format ファイル用意して統一するようにするのは有りだと思いますが、appveyor とかでチェックするのはどうかなと思います。厳しくした後にプロジェクトが廃れちゃうと（運用ルールを厳しくしたのが原因かはともかく）元も子もないです。

ですね。有用な制約もありますが、制約の付け方の匙加減は慎重に考えたいです。

以前 @KENCHjp さんがおっしゃられていた

「守れないルールが増えるのはルールが無いことよりも悪い」

の言葉が自分は好きです。

[berryzplus]

こういうのはアリの認識です。

void f( _In_z_ const char* psz) {
  auto len = strlen(psz); //←pszのNULLチェックなし、NUL終端はある前提のコード
}

ちゃんと書くなら assert(psz) するのがベターで、
strlenでなくstrnlenやStringCchLengthAを使うべきところです。
SALで宣言するならいいか、という感じになります。

別版でこういうのもあります。

void f( _Out_ int* p) {
  *p = 0; //ノーチェックで代入
}

DirectWrite関係のライブラリ実装では、こういうのを許容するルールだそうです。
NULL渡すと当然アクセスバイオレーションですが「そういう仕様」の一言で済ませるポリシー。
速度重視だとこういうコーディングスタイルもありなのかな、とか。

[kobake]

速度重視

そもそも assert って Release ビルド時には抹消されるので速度に影響は出なかったはず（と記憶）。

[berryzplus]

DirectWirteがノーチェックで書込みにいく仕様な件については、
本来ならifで分岐判定して E_INVARIDARG を返すべきところ
問答無用で AccessViolation になっていることを「速度重視だからしょうがない」と言ってます。

assertがReleaseビルドに含まれないのは正しい認識です。

[kobake]

C/C++ 系 LIB はだいたい「速度重視だからしょうがない」の思想ですね。
我々には NullPointerException なんて必要ない。AccessViolation だけあれば良い。

[beru]

多重ループ内から呼び出しされる場合等の、1回のエラーチェックに掛かる時間が1マイクロ秒以下でも積み重なると問題になるような場合は、正常なパラメータを渡して呼び出すことを前提にする方が良いと思います。エラーチェックするI/FとしないI/Fを分けて用意するのも有りかもしれないですね。

あと外部I/Fではなく内部I/F関数の場合で、範囲外のパラメータを渡す関数の呼び出し方は記述されていないのに関数内で引数のチェックをしていると実質的に無駄な事になってしまいます。

「将来的に誰かが違う呼び出し方をするコードを書くかもしれないのでそれは危ない！」という意見もありそうですが…。

[ds14050]

sakura-editor/sakura#452 (comment) でちらっと触れた件です。

『エキスパート C プログラミング』のコラムのひとつでした。

Handy Heuristic: unsigned型について一言
　不必要に unsigned 型を使って、無用の複雑さを持ち込まないこと。特に、表現する値が負になることはない（たとえば「年齢」や「国の負債総額」といったデータだ）という理由だけで unsigned 型を使わないこと。
　int のような符号付きの型を使えば、複数の型を混在させたときの「格上げ」の規則で煩わされずに済む。
　unsigned 型を使うのは、ビットフィールドやバイナリのマスクだけに限ることだ。式にキャストをかけて全オペランドを符号付きか符号なしに統一すれば、コンパイラは式の結果の型を作り出さずに済む。

この本は主としてコンパイラ(作者)の視点から書かれていて、言語(利用者)の側から書かれた本とはひと味違います。コラムの内容に関しては、これだけでは個人の見解であって、識者の客観的・多面的な評価を待ちたいところです。

C++ だと size() メンバが unsigned 型を返すので、符号なしで統一するという選択肢が視界に入ってくるんですよね。

[KENCHjp]

モデラー視点だと、正の数しか取り得ないものは正の数と定義したい。。。
それがその項目をより正しくあらわしているのなら。

[ds14050]

もっと進めて整数型をサブクラス化して、単位を型の一部に含める、値の範囲を制限するという考えがありますよね。型でプログラミングするような言語の話だったと思いますが、CLogicInt/CLayoutInt がそういう流れにあるのでしょう。

コラムに関しては、実質的に型が int しかないも同然の C ならではのものかもしれません。

[beru]

clang や gcc のように VC++ でも sanitizer で色々と検出出来ると良いですね。
VC++ から clang を呼べるとはいえ、runtime も絡むところなので多分まだ無理です。

[ds14050]

今日読んでいました『Scala 関数型デザイン＆プログラミング』という本の pp.284-285 から、contest 関数をリファクタリングする２ステップの例の抜き書きです。

---

case class Player(name: String, score: Int)

def contest(p1: Player, p2: Player): Unit =
  if (p1.score > p2.score)
    println(s"${p1.name} is the winner!")
  else if (p2.score > p1.score)
    println(s"${p2.name} is the winner!")
  else
    println("It's a draw.")

　contest 関数では、結果を表示するための I/O コードが、勝者を割り出すための純粋なロジックと結び付けられています。このロジックを winner という純粋関数として抜き出してみましょう。

def winner(p1: Player, p2: Player): Option[Player] =
  if (p1.score > p2.score) Some(p1)
  else if (p1.score < p2.score) Some(p2)
  else None

def contest(p1: Player, p2: Player): Unit = winner(p1, p2) match {
  case Some(Player(name, _)) => println(s"$name is the winner!")
  case None => println("It's a draw.")
}

まだリファクタリングの余地があります。contest 関数にはまだ役割が２つあるからです。contest は、表示するメッセージを計算した後、そのメッセージをコンソールに書き出しています。これについても純粋関数として抜き出すことが可能です。

def winnerMsg(p: Option[Player]): String = p map {
  case Player(name, _) => s"$name is the winner!"
} getOrElse "It's a draw."

def contest(p1: Player, p2: Player): Unit =
  println(winnerMsg(winner(p1, p2)))

　副作用である println がプログラムの最も外側のレイヤにのみ存在するようになったことと、println の呼び出しの内側にあるのが純粋な式であることがわかります。
　これは単純な例だから、と思っているかもしれませんが、もっと大規模で複雑なプログラムにも同じ原理が当てはまります。本書のねらいは、こうしたリファクタリングをごく自然なものに思えるようにすることです。

---

関数型言語を自分の道具としたことはなく、Scala のコードも、眺めて雰囲気を感じ取るだけですが、副作用を局所化するという考えは理解できます。

それもそのはず、sakura-editor/sakura#452 (comment) や sakura-editor/sakura#509 で示した自分の改良案に共通する特徴だからです。思わぬ援軍を見つけた気持ちでした。

なお、引用部分は第13章の導入であり、その章の眼目ははるかに理解を超えていました。

[ds14050]

sakura-editor/sakura#650 (comment)

指摘（＝修正要求）なのか、感想なのか、意図は確認したほうが良いと思います。

意に沿わない書き方はしたくないので意図的にはぐらかしています。意図を確認するなんてとんでもない。

「意図を確認するなんてとんでもない」の真意を書きます。「はぐらかす」という単語に反射的に反発を感じた人も読んでください。

「とんでもない」のは「都合が悪いからそんなことできない」という意味ではありません。「そうする意味・目的・必要がないからそうしない」というだけのことです。

根本的な価値観について確認します。他人に何かを要求すること、他人を動かそうとすることは傲慢な行いです。現代日本には奴隷も主人もなく自由人しかいませんからこれには同意してもらいます。

その上で他人を自分の思い通りにしようとするなら、それなりの準備と企みが求められます。かわいさをアピールしておねだりするのもひとつ。この場に相応しいのは、根拠を集めて理論で武装して理詰めで説得することだと自分は思っています。

「私はこう思う」という感想に対して「私はそうは思わない」は全く問題のない応答です。これが許されないならある人の考えがある人の考えに優先することになりますが、そんなルールは自分は認識していません。

「私はこうすべきだと思う」という修正要求なのであれば、なぜ自分の考えに従うべきであるかの説明があって然るべきだと思います。でなければ「自分がこうだって言うからこうなんだ」という子供らしく傲慢な駄々です。

感想も要求もどちらもあっていいし使い分ければいいんです。しかし体裁が伴っていないのに「これは修正要求であるという意図」だけを確認しても意味がないんです。

完璧にできているかは他者の評価をまたなければいけませんが、自分はコメントの「強度」に応じて真摯に対応しているつもりです。自分の意見が何にも勝るというような対応はしていないつもりです。

[berryzplus]

レビューについての考え方が根本的に違うんですね。

レビューをするときには必ずPRを出すと思うんですが、
PRはプルリクエスト（＝取り込み要求）を省略した言い方です。

考え方は自由ですが、ここに参加する時点である程度傲慢でなければならん気がします。
そして、傲慢な行為を行う以上、PRを出した人には要求の正当性を説明する義務がある気がします。
本筋と関係ないツッコミに対して、そんなもん知らんよ、と教えてあげる義務もある気がします。

レビューにコメントを書くってことは、基本的には修正要求です。
たいていの場合、確認するまでもなく修正要求だと思います。
意味無く呟いてみるとか、twitterじゃねーよ、と突っ込みたくなるわけです。

とはいえ、PRに対する評価スパンと関係のない感想を書いてしまうこともたまにはあると思います。
そういうコメントのすべてに対応が必要かというと、違う気がします。

どうしたらいい、みたいな具体的なことはゆえんのですが、
意味のあるやり取りができるようにレビューする側もされる側も気を付けていけたらいいな、と思います。

[ds14050]

berryzplus さんの書かれたことは PR の本質的な部分に関しては正しいと思います。その PR が取り込まれるべき理由であるとか、その PR がとる手段の妥当性であるとか、そういうことを PR を出した人は説明する義務があると思います。

しかし自分が先のコメントで想定していたのはコーディングスタイルに関することです(※そういう場所を選んで書きました)。

「こう書いた方が見やすい」というのは主観であり感想です。それに対して「いや私はこの方が見やすいんだ」と返すことが許されるというのが自分の考えです。＜追記＞なのでいちいちそういう指摘はしません。しかし論拠を思い付いたときに参考情報のつもりで書き連ねたらこれが大失敗でした。＜/追記＞

＜追記＞「インデントが読みにくい」という指摘に対しては「こういう観点でインデントを揃えている」という内容の返答をしました。＜/追記＞

PR の本質に関わる部分ではなく、そういうレベルのことについて書きました。

[berryzplus]

了解っす。そんなら問題ないです。

本筋と関係ないツッコミに対して、そんなもん知らんよ、と教えてあげる義務もある気がします。

なかなかやれないですけどねｗ

[KageShiron]

Lintやformatterでコードスタイルを強制するのが近年の流行なので自動化できる部分は流れに乗ってみるのもありかもしれませんね

私も個人で書いてるgoとかjavascriptとかはpre-commitフックで強制フォーマット&Lintが通らない限りコミットできないようにしてます。go fmtに関しては設定機能自体が完全に排除されてるので、もはや悩む余地すら無いという徹底ぶり。

[beru]

Lintやformatterでコードスタイルを強制するのが近年の流行なので自動化できる部分は流れに乗ってみるのもありかもしれませんね

私も個人で書いてるgoとかjavascriptとかはpre-commitフックで強制フォーマット&Lintが通らない限りコミットできないようにしてます。go fmtに関しては設定機能自体が完全に排除されてるので、もはや悩む余地すら無いという徹底ぶり。

自分の好きなコードスタイルに皆さんが合わせてくれるという事であれば諸手を挙げて賛成します。

[ds14050]

自分の好きなコードスタイルに皆さんが合わせてくれるという事であれば諸手を挙げて賛成します。

beru さんに完全同意です。つまり……(戦争だ！)

[berryzplus]

ダメだ。コーヒーブランチになってねぇ・・・ｗ

ほぼ同意で、みなさんがぼくに合わせてくれるということなら文句ありません（キリッ

[KENCHjp]

ダメだ。コーヒーブランチになってねぇ・・・ｗ

PR内では、バグじゃなければフォーマットの話はぐっと心の奥底にしまいましょうで、
このIssueでは、結果「どうでもいいかぁ」とか「あそういう意図あったのね」とか「そういう書き方もありだね」とか思う存分戦ってください(笑)