すでにマージされた PR #877 と PR #1079 を追って出す PR です。

[ds14050]

日本語では無理だったが C++ でなら理解されるかも、という「奇妙な」期待は持っていませんが、PR #1079 のコメント欄に散々書いたことの C++ 版です。

もう何度目かになる内容ですが、PR の概要です。

1. これはすでにマージされた PR 構造体比較にmemcmpを使うのをやめる #877 と PR MYDEVMODEの等価比較演算子の隠れバグを修正する #1079 を追って出す PR です。
2. 構造体比較にmemcmpを使うのをやめる #877 と MYDEVMODEの等価比較演算子の隠れバグを修正する #1079 にはともに MYDEVMODE 構造体の比較演算に関する実装コードとテストコードが含まれています。
3. MYDEVMODEの等価比較演算子の隠れバグを修正する #1079 で加えられた実装の修正を取り消し、構造体比較にmemcmpを使うのをやめる #877 の実装を回復します。
4. 両方の PR に含まれていたテストの枠と構成を借用しましたが、
   1. 冗長な記述を簡素化しました。
   2. (時間がかかるらしい)デステストを廃止しました。ASSERT_DEATH はテストコードがアクセス違反を起こせることをテストするために存在していましたが、実際にアクセス違反を起こさなくてもテストはできます。
   3. テストが確かめる仕様を整合性のあるものに改めました。

「整合性」について

MYDEVMODE の文字列メンバの入出力コードを見たところ、読み込みでも書き込みでもヌル終端されないケースは想定していないようでした。「落ちる」と書いてありましたし、実際に読み書き共に落ちたり暴走したりしそうでした。

ここから理解されることは、文字列メンバがヌル終端されないケースは即刻死につながる、徹底的に排除されるべき例外的状況だということです。入出力コードが実質的な assert として機能します。

#1079 のテストが規定する仕様と、当初は「気がついていませんでした」が #877 にも織り込まれていた仕様を「悪しき仕様」と呼ぶ理由について(これまた何度目かになりますが)書きます。

#1079 に含まれるテストのすべてと、#877 に含まれる一部のテストは、文字列メンバがヌル終端されていない状況を温存し、尊重しなければ通らない ものになっています。文字列メンバがヌル終端文字列であるという、かつては存在していた前提を捨てさせることを要求するものになっています。

そのような要求は先ほど挙げた入出力コードと整合しませんし、テストの存在が枷となって、例外的状況を排除するような将来の改善(たとえばヌル終端を書き込んで長すぎる文字列を切り詰めるような処理)を妨げます。

---

私は #1079 には疑問を付けましたが、#877 は実装もテストもそのままの形で生き残るものだと最後まで思っていました。「悪しき仕様がテストに織り込まれていた」と気がつくまでは。

#877 とは違い、#1079 は「何をしても落ちない」ことを助ける内容ではありません。勘違いは「しょうがない」ことかもしれませんが、他人の責任までは負えません。

[AppVeyorBot]

✅ Build sakura 1.0.2409 completed (commit d0de8ac841 by @ds14050)

[berryzplus]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[berryzplus]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[beru]

問題無いと思います。

個人的には不定値が心配なら MYDEVMODE にユーザー定義コンストラクタを追加して実装は memset で一気にゼロ初期化して、構造体の等値比較は memcmp で行ってしまう C言語ライクな実装の方が楽だしそれで大きな問題は無さそうに思います。

しかしそんな事を公言してしまうと、メキシコ人の漁師的な思想を持っていると思われて説教をされたり「田舎に帰って畑でも耕してろ」と罵倒されかねないので前言を撤回します。

[ds14050]

前置き GitHub にサポートされない古いブラウザしかインストールできないので唯一コメントできるここにコメントしています。

比較演算子の定義場所についてです。メンバである必要がないので非メンバ非friend のフリー関数として定義しました。public である必要がなければ private にするのと同じような理由です。

演算子をメンバとして定義すると互換性のある異なる型との間で交換法則が成り立たせられなくなるので、二項演算子をメンバとして定義するものとは考えない方がいいと思います。

演算子の引数が MYDEVMODE 型なので比較演算子は依然 MYDEVMODE を構成するコードの一部と見なされます。この結論は ADL に関連してなされた議論をふまえたものらしいです。本で読みました。

(無名)名前空間で括るまでがセットかもしれませんが、名前空間を積極的に利用しているわけではないのでそれは省略しました。

[ds14050]

不定値が心配なら

「不定値」とはヌル終端されない外れ値のことを指していますか？

特に心配はしていません。しかし外れ値は外れ値として排除されるべきものであると考えています。それを妨げるようなテストの存在により、MYDEVMODE の比較演算子だけが他のコードと整合性のとれない引数の取り扱いをさせられていました。

矛盾がないこと、整合性がとれていることが、絶対に必要なことだと考えています。でなければその場しのぎのつぎはぎの末にあっという間に手に負えなくなります。

memset で一気にゼロ初期化して、構造体の等値比較は memcmp で行ってしまう

それは #877 のレビューで言うべきことなんですよね。

メンバを比較する比較演算子の実装を自分はイチから書いたわけではありませんが、それでも、これは人間がやる作業ではないなと感じました。だから memset/memcmp 案は否定できませんが、自分が手を動かすわけではないのだからいいではありませんか。

自分は、間違えなければ、害がなければ、思うようにやればいいとの考えで他の人の活動を見ていました。効率が悪くても、おかしなやり方をしていても、どっちでもいいならどっちでも構いません。理想の追求は個人活動です。強要はできませんし、各人が各人の学習段階にいて、その段階でできることとできないことがあります。望む通りにやり遂げてもらうことが第一です。

こういう PR を出すということは、意図するとせざるとに関わらずこういうメッセージを帯びます。「お前には無理だ。俺がやる」。メッセージが妥当かどうかは関知しませんが、避けたい振る舞いです。避けられませんでした。

[berryzplus]

いいんかいっ！ 😄

個人的には、メンバ演算子をグローバル演算子に変更する行為が思考停止レベルで「ないわ」だったのでまずそこを突っ込みましたが、他にも懸念点はいくつかあります。
（あ、 #1105 (comment) 見て言わんとしていることは分かりました。

パフォーマンスが落ちる方向の変更が２つ入ってますよね。

1. operator == (...) の inline が解除されている。
2. 冒頭で行っていた同ポインタ判定が除去されている。

頻繁に比較するものじゃないから「実質問題なし」と言えなくもないですけど。

---

あとは、文字列メンバの比較で最終要素を無視してよい根拠が分からんです。

char bufA[5] { "xxxx" };
char bufB[6] { "xxxxx" };
bool ret1 = (0 == strncmp(bufA, bufB, _countof(bufA) - 1)); //結果はtrue
bool ret2 = (0 == strncmp(bufA, bufB, _countof(bufA))); //結果はfalse

ret1が最終要素を無視した比較、ret2が最終要素を考慮した比較です。
左辺側のNUL終端が保証されていたとして、最終要素を無視してよい根拠が分からんとです。

[beru]

不定値が心配なら

「不定値」とはヌル終端されない外れ値のことを指していますか？

MYDEVMODE 構造体型ですが、単に変数宣言しただけだと値が不定値になる事を指しています。

発端の #877 (comment) では、構造体内のパディング部分の差異で等価判断を正しく行えなくなることが …と書かれていますがそれも「不定値」カテゴリーに含めてしまって良いと思います。実際にそういうケースが今 MYDEVMODE で起きているのかどうかはともかくとして。

特に心配はしていません。しかし外れ値は外れ値として排除されるべきものであると考えています。それを妨げるようなテストの存在により、MYDEVMODE の比較演算子だけが他のコードと整合性のとれない引数の取り扱いをさせられていました。

矛盾がないこと、整合性がとれていることが、絶対に必要なことだと考えています。でなければその場しのぎのつぎはぎの末にあっという間に手に負えなくなります。

ユニットテストって人間が問題を見逃しがちだからプログラムに自動検査させようというのが本来の趣旨のような気がしますが、なんかテストコード側で仕様を定義っていうのがちょっと本末転倒感が有る気がします。実装量に見合うようにテストコードの量を増加するのが現実的に厳しいんじゃないかと思うので、もし不具合が出たらその時に手を入れて直すといういいかげんなやり方の方が楽かなと…。

memset で一気にゼロ初期化して、構造体の等値比較は memcmp で行ってしまう

それは #877 のレビューで言うべきことなんですよね。

まぁそれは確かに。

メンバを比較する比較演算子の実装を自分はイチから書いたわけではありませんが、それでも、これは人間がやる作業ではないなと感じました。だから memset/memcmp 案は否定できませんが、自分が手を動かすわけではないのだからいいではありませんか。

人手で繰り返し書くのはきついですよね。Xマクロで少し楽を出来る場合も有りますが。。

あと下記のライブラリを使う手もありそうです。
https://github.com/cbeck88/visit_struct

自分は、間違えなければ、害がなければ、思うようにやればいいとの考えで他の人の活動を見ていました。効率が悪くても、おかしなやり方をしていても、どっちでもいいならどっちでも構いません。理想の追求は個人活動です。強要はできませんし、各人が各人の学習段階にいて、その段階でできることとできないことがあります。望む通りにやり遂げてもらうことが第一です。

こういう PR を出すということは、意図するとせざるとに関わらずこういうメッセージを帯びます。「お前には無理だ。俺がやる」。メッセージが妥当かどうかは関知しませんが、避けたい振る舞いです。避けられませんでした。

MYDEVMODE の比較に関しては、個人的には（あまり印刷しないので）有る意味どうでも良いかなという気はあります。むしろ CPrintPreview::OnPrint で比較せずに PostMessageToAllEditors を呼び出しちゃってもそこまで致命的な事にはならないだろうし、それなら比較の細かい実装をタネに言い争いしなくても良いんじゃないかなと思ってます。

[berryzplus]

個人的には不定値が心配なら MYDEVMODE にユーザー定義コンストラクタを追加して実装は memset で一気にゼロ初期化して、構造体の等値比較は memcmp で行ってしまう C言語ライクな実装の方が楽だしそれで大きな問題は無さそうに思います。

ああ、ちなみに #877 は、まさしくそんな実装になってた部分が SonarQube の detector に怒られてたのを「試しにモダンな感じに実装してみました（キリッ」なPRでした。

しかしそんな事を公言してしまうと、メキシコ人の漁師的な思想を持っていると思われて説教をされたり「田舎に帰って畑でも耕してろ」と罵倒されかねないので前言を撤回します。

別にそれでもいいんじゃね？と思います。
まー、他人に強要しなければ、ですが。

矛盾がないこと、整合性がとれていることが、絶対に必要なことだと考えています。でなければその場しのぎのつぎはぎの末にあっという間に手に負えなくなります。

えっ、もしかして「テストコードはアプリコードが想定しない条件をテストすべきじゃない」とか思ってませんか？

• テストコードとアプリコードに矛盾がない
• テストコードとアプリコードの整合性が取れている

現場思考でいくとその状況は、テスト担当者仕事しろ、です。

「絶対に必要なこと」と言ってますが、何故必要なのか教えてほしいです。

テストコードは、あらゆる条件をテストすべきだと思います。

基本的にはアプリコードが想定する条件のテストが中心になりますが、条件のピックアップに使うのは実装ではなく仕様です。もしもテストコードがアプリコードの実装に合わせたテスト条件のみを判定していたら、アプリ実装者が想定しない条件はテストされないことになっていまいます。それでは困るのでテストは仕様から起こします。サクラエディタには仕様がないので、実装から大枠の仕様を想定してそれを元にテストを書いています。

アプリ実装で想定している条件とテスト実装で想定する条件は違って良いと思います。
アプリ実装は機能の実現が目的で、テスト実装は品質の担保が目的です。
目的が違えば、同じ人が同じ機能を見ても気にするポイントが変わってくるはずです。
もし「それが同じになることが絶対に必要だ」とするなら別な軸の何かに従って行動するしかない気がします。ぼくはそれを知らんので、その理由を知りたい感じです。

[beru]

個人的には不定値が心配なら MYDEVMODE にユーザー定義コンストラクタを追加して実装は memset で一気にゼロ初期化して、構造体の等値比較は memcmp で行ってしまう C言語ライクな実装の方が楽だしそれで大きな問題は無さそうに思います。

ああ、ちなみに #877 は、まさしくそんな実装になってた部分が SonarQube の detector に怒られてたのを「試しにモダンな感じに実装してみました（キリッ」なPRでした。

#877 のコメントにあるリンクが permalink じゃないのか辿ってもそれっぽい指摘を探せませんでした。

元の実装は MYDEVMODE のユーザー定義コンストラクタなどで初期化はやっていないですね。
なので memcmp だと危ういかも、という感は有ります。

[berryzplus]

#877 のコメントにあるリンクが permalink じゃないのか辿ってもそれっぽい指摘を探せませんでした。

ホンマや！辿れん！
ミスった...orz

[beru]

個人的には、メンバ演算子をグローバル演算子に変更する行為が思考停止レベルで「ないわ」だったのでまずそこを突っ込みましたが、他にも懸念点はいくつかあります。

https://qiita.com/rinse_/items/9d87d5cb0dc1e89d005e#equality-compare

二項演算子はグローバル関数教 の信徒の方が仰るには等値比較演算子はグローバル関数であるべきのようです。理由については記述されていませんが何やら拷問官の方が焼きごての準備を始めるくらいとても大事な事のようです。

比較する LHS と RHS の型が異なる場合にメンバ関数だと対応出来ないかなと思いますが、今回は MYDEVMODE 型同士の比較なのでメンバ関数でも良い気はします。

こっちじゃないと駄目なんだ、という際に論理的に明確な理由または圧力が無い場合は、他人を説得するのが難しいですね。

あとは、文字列メンバの比較で最終要素を無視してよい根拠が分からんです。

char bufA[5] { "xxxx" };
char bufB[6] { "xxxxx" };
bool ret1 = (0 == strncmp(bufA, bufB, _countof(bufA) - 1)); //結果はtrue
bool ret2 = (0 == strncmp(bufA, bufB, _countof(bufA))); //結果はfalse

ret1が最終要素を無視した比較、ret2が最終要素を考慮した比較です。
左辺側のNUL終端が保証されていたとして、最終要素を無視してよい根拠が分からんとです。

構造体の同じメンバー（固定長の文字配列型）を比較する場合は両方とも配列長が同じなので上記のコード例だとどうかなと思います。

[berryzplus]

ちなみに MYDEVMODE にテストが必要かと言うと、答えはNOです。

PRがこのままマージされても実害はないと思っています。

ただ、後学のために最終要素を無視して構わない根拠とテストとアプリが整合性を保つべき理由は確認しておきたいです。

[berryzplus]

二項演算子はグローバル関数教 の信徒の方が仰るには等値比較演算子はグローバル関数であるべきのようです。理由については記述されていませんが何やら拷問官の方が焼きごての準備を始めるくらいとても大事な事のようです。

「駆逐してやる！一匹残らず！」とどこかの漫画の厨二なセリフを吐きたくなる感じでした。
ま、でもグローバル真理教の人でも型が比較演算子を持つことを否定しているわけではなさそうですね。

構造体の同じメンバー（固定長の文字配列型）を比較する場合は両方とも配列長が同じなので上記のコード例だとどうかなと思います。

う～む。アプリ実装者の視点から「最終要素を無視してもよい理由」の説明にはなると思いました。しかしテスターの視点からすると、やっぱり「何で？」です。安全にアクセスできる要素が５個あると分かっているのに、４個までしかチェックしない意味が分からんです。アクセスできる要素が５つあるなら５つの要素それぞれで差異を検出できるかチェックするのがテスターの責務だと思います。で、理不尽な要求だと思いながらコードを直すパターンですね・・・。

ちなみに #877 の比較演算子を作るのにかかった時間は5分くらいです。
こういうときのためのサクラエディタ、メンバをガサッとコピペして正規表現置換、発☆動。

[ds14050]

最終要素を無視して構わない根拠

扱っている文字列メンバが「ヌル終端文字列」だからです。この前提はアプリを通して共有されているものであり、守るべき条件です。

テストは仕様から起こします。

文字列メンバがヌル終端文字列であることを「仕様」にしてテストを書きました。

えっ、もしかして「テストコードはアプリコードが想定しない条件をテストすべきじゃない」とか思ってませんか？

アプリコードに不備があり不具合を出したケースをテストにしたものが以降もリグレッションテストとして手戻りを検知するのに使えるわけですよね。このときテストはアプリが「想定していなかった条件」を与えて、それでも正しい結果が返ることを確認するために実行されるわけです。それがダメとは一言も言っていません。

想定外の条件でどのように振る舞うかが分岐点になりました。何が「正しい結果」であるかです。

アクセス違反のような致命的な不具合を起こさないことを確認しながら、「扱っているのがヌル終端文字列であるという仕様」をアプリコードが放り捨てないでもいいようなテストを書きました。

テストとアプリが整合性を保つべき理由

整合性を保つべきはアプリコード全体です。比較演算子だけが、扱っているのがヌル終端文字列ではないかのように振る舞うことが不整合です。

[ds14050]

それなら比較の細かい実装をタネに言い争いしなくても良いんじゃないかなと思ってます。

空気を悪くして申し訳ありませんでした。

#1079 の動機が(潜在)バグ修正だったからカッとなってしまいました。誤った事実認識に基づく誤った行動に見えたのです。冤罪を被せられた第三者はいなかったと後で判明したわけですが……。

[ds14050]

こっちじゃないと駄目なんだ、という際に論理的に明確な理由または圧力が無い場合は、他人を説得するのが難しいですね。

そうですね。他人に要求するときには根拠を用意すべきですね。この PR は、成り行きからですが、他人のお株を奪うつもりで私が書いたものなのでこうなりました。

[berryzplus]

テストは仕様から起こします。

文字列メンバがヌル終端文字列であることを「仕様」にしてテストを書きました。

構造体の一致/不一致を判定する機能の仕様に、
メンバがヌル終端文字列であることを織り込む。
　↓　（その結果としての仕様）
比較対象の構造体のいずれかが不正ならば、比較結果も保証されない。（バグではなく、仕様。）
なお、ヌル終端文字列の規則を遵守するのは当然のことなので、検証関数は用意しない。

そういう仕様を見かけることもあるので、絶対におかしいとは言えません。
外部から不正値が入る余地はないので、この実装で現状困ることもないと思います。
検証関数も、必要になったときに入れれば問題ないです。

ただ、比較演算子の素直な仕様（一致するかしないか）を採用できないくらいに、メンバにヌル終端文字列を含むということが特別なのか？という気はします。

[ds14050]

@berryzplus さんの頭の中では抽象化の階層と主従の関係が逆転しているように感じます。文字配列とヌル終端文字列の関係です。

文字配列はヌル終端文字列を表現するための道具です。文字配列としての比較結果をヌル終端文字列としての比較結果より優先するならば、それこそ構造体全体を (w)memcmp で比較して済ませてしまえば良いのです。

比較演算子の素直な仕様（一致するかしないか）

「素直」について思考停止せずに考えてください。誤っています。ご自分で書きました。(ブラックボックス)テストは仕様に対して書くと。そこは間違っていないと自分も思います。仕様化された抽象を扱っています。バイト列や文字配列を扱っているのではありません。

比較対象の構造体のいずれかが不正ならば、比較結果も保証されない。（バグではなく、仕様。）

これも「素直」に短絡思考に走るのではなく、よく考えてください。@berryzplus さんが保証しようとしている比較結果は、文字配列としての比較結果です。

文字配列とヌル終端文字列の違いについてよーくよーく考えてください。

N 要素の文字配列の比較において N+1 文字目の比較結果を考えることがナンセンスなのと全く同じに、N 要素の文字配列で表現されるヌル終端文字列において、存在しない N 文字目の比較結果を考えることは意味のないことです。N 文字目を比較した結果を「保証」するということは、「ヌル終端文字列であるという仕様」を放棄するか、文字配列を拡張するかしなければできないことです。

---

ここから話がさらにややこしくなりますが、「ヌル終端されていないヌル終端文字列」を想定するということがパラドキシカルではあります。だから何がテストで想定すべき「正しい結果」であるかは、「こうする」と決めてしまわなければ本来は決まらないことです。

入出力コードの態度は「想定しない」であり、「ヌル終端されていないヌル終端文字列」の存在は世界の破滅を意味します。

@berryzplus さんが書いたテストコードとアプリコードは「ヌル終端文字列として扱うことをやめる」ものでした。

しかし比較演算子だけが仕様を無視したそういう態度をとることは不整合であるし、実際にヌル終端されないケースというのが「原理上存在が否定できない」というレベルでしか実在しないものであるので、「ヌル終端までが文字配列に収まるように切り詰めた」という想定で得られる比較結果を、テストがアプリに要求するものとしました。おかげでアプリコードは以前と変わらずヌル終端文字列を操作しています。

[berryzplus]

やっぱり理解できないっす。

これも「素直」に短絡思考に走るのではなく、よく考えてください。@berryzplus さんが保証しようとしている比較結果は、文字配列としての比較結果です。

文字配列とヌル終端文字列の違いについてよーくよーく考えてください。

N 要素の文字配列の比較において N+1 文字目の比較結果を考えることがナンセンスなのと全く同じに、N 要素の文字配列で表現されるヌル終端文字列において、存在しない N 文字目の比較結果を考えることは意味のないことです。N 文字目を比較した結果を「保証」するということは、「ヌル終端文字列であるという仕様」を放棄するか、文字配列を拡張するかしなければできないことです。

N文字のヌル終端文字列って、
N+1文字目にヌルが入ってる（＝終端されている）文字配列ですよね？
N+1文字目のヌルを含めて「ヌル終端文字列」って言うんじゃないですかね？
だから「N+1文字目が配列の最終要素である場合のみ」先頭N文字だけを見る仕様の妥当性に納得がいかんのです。wcsncmpを使う以上、最終要素以外はヌル終端を含めて比較しちゃいますから。

先に書いた通り、ある意味で仕様として正しいことは理解しています。
んで、単純比較に関しては「ヌル終端文字列」を考慮しないほうが楽になると思っています。
NULL終端文字列を守れないヤツは氏ねよ、って意見ですが、そのほうが楽だと思ってます。

[ds14050]

N文字のヌル終端文字列って、N+1文字目にヌルが入ってる（＝終端されている）文字配列ですよね？

やり直しです。「N 要素の文字配列で表現されるヌル終端文字列」と書きました。それは「ヌル文字以外の文字で構成される、最長で N-1 の長さを持つ可変長文字列」です。

もう一度よーくよーく考えてください。

[ds14050]

wcsncmpを使う以上、最終要素以外はヌル終端を含めて比較しちゃいますから。

この状況は２通りの解釈ができます。

1. 一方に現れたヌル文字と他方のヌルでない文字を比較した結果が異なるから -1 もしくは +1 が返る。
2. 一方が他方のプリフィックスになっていることが判ったから -1 もしくは +1 が返る。

1 は実装寄りの解釈で、2 は理論寄りの解釈と言えます。1 は方便であり、2 の仕様を効率的に実装する手段です。

[ds14050]

単純比較に関しては「ヌル終端文字列」を考慮しないほうが楽になると思っています。

元が (w)memcmp なのだから実装の手抜きに目くじらを立てることはできません。しかしテストが邪魔になります。

これはもう書いたことですが、一連の２つの PR が導入したテストがあることによって、「楽だからヌル終端文字列を考慮しない」ではなく、「ヌル終端文字列を考慮してはいけない」状況が発生しています。

楽をするなら、邪魔になるテストを追加する手間も省いてもらわなくては困ります。

[berryzplus]

やり直しです。

態度がでかい！アフォの分際で。
と、アフォのおいらが言うと滑稽ですね。

「N 要素の文字配列で表現されるヌル終端文字列」と書きました。
それは「ヌル文字以外の文字で構成される、最長で N-1 の長さを持つ可変長文字列」です。

どうも「ヌル終端文字列」が意味するものが C++ の「文字列」（＝NULで終端される文字のシーケンス）と違うような気がしてきています。

コンピュータで文字列の長さを表現する手法は、基本的に二択です。

• 計算済みの長さと文字列ポインタを別に持つ。　例) std::wstring、WindowsのBSTR
• 文字列の最後に終端記号を置いて毎回計算する。　例) C言語のヌル終端文字列

NUL終端を選んだ時点で、終端のNULを確認するのは責任だと思います。
NUL終端文字列で要素数が決まっているときに取るべき対応は「無視」じゃないと思います。
最大要素数がN個と決まっているならstrnlenを使うだけです。
strnlenが引数と同値を返した場合(=NUL終端がなかった)の対応をどうするかは好みですが。

[ds14050]

@beru さんが書きました。

なんかテストコード側で仕様を定義っていうのがちょっと本末転倒感が有る気がします。実装量に見合うようにテストコードの量を増加するのが現実的に厳しいんじゃないかと思うので、もし不具合が出たらその時に手を入れて直すといういいかげんなやり方の方が楽かなと…。

テストで仕様を定義するというよりは、テストにより定まる仕様がある、という風に考えています。

テストが assert によって想定結果と実行結果を突き合わせることで、「～でなければいけない」「～であってはいけない」という形でプログラムの外形が決められてしまいます。それを仕様と呼んでいます。

テストが確認することが仕様のすべてではないし、仕様のすべてをテストに落とし込むべきだとも考えてはいませんが、少なくともテストが仕様の一部を定めます。これは意思の表明ではなく現実がそのように解釈されるということで、否定できないことです。

テストによって仕様が定まるから、テストは仕様に対して書きます。それを徹底することで、仕様を変えない実装の修正があってもテストはそのまま通用します。できていなければ、「PR #1093 等価比較演算子の実装がラクになる方法を探ってみる」で見られたように、何かするたびにテストの修正が同時に必要になります。テストが修正箇所を増やすだけの枷になります。

この理解が１つ前のコメントの末尾につながります。>「楽をするなら、邪魔になるテストを追加する手間も省いてもらわなくては困ります。」

私は徹底した怠け者です。(潜在バグ修正の名の下に)余計な手間を増やすだけの手間をかける行為(PR #1079)が許せなかったのです。

[beru]

矛盾がないこと、整合性がとれていることが、絶対に必要なことだと考えています。でなければその場しのぎのつぎはぎの末にあっという間に手に負えなくなります。

一部の記述が仮に駄目になったとしても、それが一気に全体に波及して手に負えなくなるかというと、メンバーみんな高齢化しているのでそこまで元気にコードを量産出来ないと思います… …。:older_man: なのでご安心下さい。

MYDEVMODE の等値比較演算子で、固定長ワイド文字配列のメンバーを比較する際に、最後の要素まで含めた長さにするのかしないかについては、挙動的にどちらでも同じ結果になるのならこだわるべきでは無いと思います。自転車置き場の議論じゃないでしょうか？どちらが折れるか、こちらの論理を認めさせるか、みたいなバトルになっていますが、そもそもどちらも同じ入力で同じ戻り値になるのなら本質的に優劣付け難い気がします。

[beru]

#1110 との兼ね合いもあるので特に異論が無いならそろそろ Merge してしまいますね。

[berryzplus]

いいですよ。実害はでないと思いますんで。

[berryzplus]

それはそうと・・・

「ヌル終端がなかったからヌル終端しておいたよ」というのが私の態度です。

「無かったから」は語弊があると思います。見てないんだから。
検査範囲に最終要素を含めないのは「なくても知らんよ」を意味すると思います。
「無視していいのか？」と言ってるのは、そう読めたからです。

実際には比較演算子が書き込みを行うのは出過ぎた行為に思えるので、

const修飾された変数に書き込みを行うのは禁止事項です。

[berryzplus]

見落としていました。

この構造体はサクラエディタ最大のグローバル変数DLL_SHAREDATAの一部でした。
このデータ領域は、サクラエディタでない任意のプロセスから自由に書き込みが行える代物なので、NUL終端のローカルルールが守られる保証はありません。

したがって、利用者全員がNUL終端規則を守ることを前提に変更を許容することができなくなりました。対策は _countof(xxx) - 1 の -1 を削除することです。

[berryzplus]

#1110 との兼ね合いもあるので特に異論が無いならそろそろ Merge してしまいますね。

一応、明示で追記しときます。「いいんじゃない？」は取り消しになりました。

[beru]

この構造体はサクラエディタ最大のグローバル変数DLL_SHAREDATAの一部でした。
このデータ領域は、サクラエディタでない任意のプロセスから自由に書き込みが行える代物なので、NUL終端のローカルルールが守られる保証はありません。

したがって、利用者全員がNUL終端規則を守ることを前提に変更を許容することができなくなりました。対策は _countof(xxx) - 1 の -1 を削除することです。

サクラエディタでないプロセスから読み書きというのは対応要件の範疇外じゃないでしょうか？

CShareData::InitShareData メソッドで CreateFileMappingW を使って名前付き共有メモリを作成していますが、その名前は GSTR_SHAREDATA マクロで作成するものです。

たまたま一致という事は有りえない事ですよね？

[berryzplus]

サクラエディタでないプロセスから読み書きというのは対応要件の範疇外じゃないでしょうか？

他プロセスによる書き込みは対応要件外としてよいです。
ただし、それで否認を取り消せるかというとそうではないです。

超巨大オブジェクトの一部と分かった、ということはMYDEVMODEよりもアドレス的に前方にある全てのメンバのバッファオーバーフローがMYDEVMODEの比較演算子の挙動に影響しうる、ということだと思います。MYDEVMODE構造体メンバのすべてがszルールを守っている事実だけでは、もはや安全性を担保するには不十分となった認識です。

もっとも、アドレス的に前方にあるすべての配列系メンバでバッファオーバーフローが起きないことを証明できるならば問題はないのかも知れません。

[berryzplus]

あ、安全性って最終要素がNULじゃなかった場合に違いを検出できないって問題のことです。

[ds14050]

利用者全員がNUL終端規則を守ることを前提に変更を許容することができなくなりました。

@berryzplus さんの方針がそれであるならそちらへ向かうのもいいでしょう。入出力コードを含めたすべての場所で、ヌル終端文字列を扱っているという前提を捨て去ったコードが書かれるべきだというわけです。私は、それがメリットのない非現実的な対応であると思います。ヌル終端されていなくても落ちないという、#877 のコードで十分だと思います。

[beru]

超巨大オブジェクトの一部と分かった、ということはMYDEVMODEよりもアドレス的に前方にある全てのメンバのバッファオーバーフローがMYDEVMODEの比較演算子の挙動に影響しうる、ということだと思います。MYDEVMODE構造体メンバのすべてがszルールを守っている事実だけでは、もはや安全性を担保するには不十分となった認識です。

もっとも、アドレス的に前方にあるすべての配列系メンバでバッファオーバーフローが起きないことを証明できるならば問題はないのかも知れません。

バッファ後方の境界越え書き込みのバッファオーバーフローだけでなく、バッファ前方の境界越え書き込みのバッファーアンダーフローもあるのと、連続書き込みで隣接バッファを壊してしまう以外にアドレス計算の不具合等で本来の位置から外れてあらぬところから書き込まれるという事も考えられます。

境界越えの書き込みによる意図しない不具合については、問題のスコープがこのPRに留まらないと思うので、それを問題にしたいなら別Issueで議論する方が良いと思います。

C言語のプログラムって文字列のNUL終端規則は基本的に守る＆守られている事を前提に記述するもののような気がします。もしそうなっていなかった場合に不正動作しないように対処したいというのは安全を考えると気持ちが分からなくもないですが、記述の簡易さを損ねると思います。

なお実行時に動的な検査処理を入れるのも対処方法の一つとしてあると思いますが、処理速度への影響を考えるとデバッグビルドに限定した方が良いと思います。

[beru]

あ、安全性って最終要素がNULじゃなかった場合に違いを検出できないって問題のことです。

文字列として扱う固定長配列の領域内に終端文字が存在しないという事はイレギュラーケースですよね？それなら正常処理ではなく異常処理として対処が必要なのか、必要ならばどう振る舞うべきか、という話にするのはどうでしょうか？

終端文字まで比較するバッファ長を伸ばすかどうかを延々と議論するよりかはまだ終着点を見出しやすいような気がします。

[berryzplus]

結局、 #1105 (comment) に書いてることのうち、片方について納得のいく回答を貰ってない気がしています。

ただ、後学のために最終要素を無視して構わない根拠とテストとアプリが整合性を保つべき理由は確認しておきたいです。

• 最終要素を無視して構わない根拠　＝　仕様として保証されるので見ません。←納得いかない。
• テストとアプリが整合性を保つべき理由　＝　保つべきはアプリ内の整合性です。

両者の主張をまとめると以下の通りです。

• 主張1. NUL終端文字列同士の比較でサイズが固定である場合、最終要素はNULか1つ以上のNULが現れた後の文字になることが仕様により保証されるので無視しても構わない。
• 主張2. NUL終端文字列同士の比較は、先頭要素からNULが現れるか最終要素に達するまでを比較する。実際にNUL要素があるかどうかは考慮しない。

入出力コードを含めたすべての場所で、ヌル終端文字列を扱っているという前提を捨て去ったコードが書かれるべきだというわけです。私は、それがメリットのない非現実的な対応であると思います。ヌル終端されていなくても落ちないという、#877 のコードで十分だと思います。

ぼくは、NUL終端文字列への文字列コピーでNUL終端し忘れるヤツは「氏ねよ」派です。

従って、最終要素でNUL終端された「正しい文字列」と最終要素にゴミが入った「正しくない文字配列」とを「等しい」と判定するこの変更を許容したくありません。

これはメリットじゃないですかね？仕様的にありえないから。

[beru]

最終要素でNUL終端された「正しい文字列」と最終要素にゴミが入った「正しくない文字配列」とを「等しい」と判定するこの変更を許容したくありません。

これはメリットじゃないですかね？仕様的にありえないから。

それを言ってしまうと、「仕様的にあり得ない NUL 終端されていない正しくない文字配列」と「仕様的にあり得ない NUL 終端されていない正しくない文字配列」を文字列として比較出来てしまう事は問題無いんでしょうか？

イレギュラーケースにも対処したい気持ちも分かりますけど、通常処理とは分けて考えた方が切り分けが楽じゃないかなぁと思います。

[berryzplus]

それを言ってしまうと、「仕様的にあり得ない NUL 終端されていない正しくない文字配列」と「仕様的にあり得ない NUL 終端されていない正しくない文字配列」を文字列として比較出来てしまう事は問題無いんでしょうか？

そうですね。問題あるのでグローバル演算子やめましょう！

ってそういう話じゃないですよね。

イレギュラーケースにも対処したい気持ちも分かりますけど、通常処理とは分けて考えた方が切り分けが楽じゃないかなぁと思います。

なんか話が噛み合ってない気がします。

• 最終要素を見ない　＝　0 == wcsnsmp(a, b, sizeof(a) -1)
• 最終要素も見る　　＝　0 == wcsnsmp(a, b, sizeof(a))

です。

NUL終端の想定を捨て去ったコードと言われるのは「？」です。
キッチリNUL終端を想定しつつ、やんちゃボーズがNUL終端しわすれても問題が起きないようにしようぜ？って言ってるだけのつもりです。

[beru]

それを言ってしまうと、「仕様的にあり得ない NUL 終端されていない正しくない文字配列」と「仕様的にあり得ない NUL 終端されていない正しくない文字配列」を文字列として比較出来てしまう事は問題無いんでしょうか？

そうですね。問題あるのでグローバル演算子やめましょう！

ってそういう話じゃないですよね。

そうですね、Javaを見習ってなるべくグローバル変数と関数（namespace内に限定しようがNG）を使わない縛りプレイをするのも良いかもしれないですね。OOP原理主義者はシングルトンやstatic methodも使わないという話を読んでネタかな…と思いましたが。

イレギュラーケースにも対処したい気持ちも分かりますけど、通常処理とは分けて考えた方が切り分けが楽じゃないかなぁと思います。

なんか話が噛み合ってない気がします。

* 最終要素を見ない　＝　`0 == wcsnsmp(a, b, sizeof(a) -1)`

* 最終要素も見る　　＝　`0 == wcsnsmp(a, b, sizeof(a))`

です。

NUL終端の想定を捨て去ったコードと言われるのは「？」です。
キッチリNUL終端を想定しつつ、やんちゃボーズがNUL終端しわすれても問題が起きないようにしようぜ？って言ってるだけのつもりです。

比較する左辺と右辺のうち片方もしくは両方がNUL終端し忘れた場合に、等値比較が最終要素を見るようにする事で解決される問題は、「同一のバイト列かどうかの判定が厳密になる」だけじゃないでしょうか？

NUL終端し忘れていた場合にその他の箇所がそもそも正常に動作するのかが心配になってしまいます。そして #1079 (review) に戻ってループ!

[ds14050]

このコメントは理解してもらえましたか>@berryzplus さん

私は berryzplus さんの意見を尊重しています。これです

このデータ領域は、サクラエディタでない任意のプロセスから自由に書き込みが行える代物なので、NUL終端のローカルルールが守られる保証はありません。

したがって、利用者全員がNUL終端規則を守ることを前提に変更を許容することができなくなりました。

NUL終端規則が守られることを前提としたコードが許容できないという berryzplus さんの意見です。領域外アクセスで落ちたりしないのはもちろん、それだけでは不十分だという意見です。

[beru]

まぁ何を言いたいかというと NUL 終端し忘れていた場合が心配だから（ちなみに自分は心配りしてません）といって「文字列比較時に最終要素も見る」というようにしておけば十分なのかというとそうは思わない、という事です。もしそういうケースに対処したいなら異常系の仕様を決めて、バッファ内容が正常かどうかを確認する記述を別途用意する方が良いと思います。「いやそんなかったるい事は不要でとにかく最終要素まで見れば問題は解決するんだ」と言われたら「この人映画のエンドクレジットまでしっかり見る人なのかな…」と思ってしまいます。

[berryzplus]

このコメントは理解してもらえましたか>@berryzplus さん

たぶん、見ましたよ。

利用者全員がNUL終端規則を守ることを前提に変更を許容することができなくなりました。

@berryzplus さんの方針がそれであるならそちらへ向かうのもいいでしょう。入出力コードを含めたすべての場所で、ヌル終端文字列を扱っているという前提を捨て去ったコードが書かれるべきだというわけです。私は、それがメリットのない非現実的な対応であると思います。ヌル終端されていなくても落ちないという、#877 のコードで十分だと思います。

「いいでしょう。」って書いてたので「諦めたのね」と理解しました。
終結済み PR が close されないのはいつものことなので、それでよいと思っています。
まー、そのうち close しちゃうかも知れませんが。

こちらとしても、意外と人の意見に聞く耳持たないわけではないんです。
駄々っ子のように「やなのやなのやなの・・・」はしますけど 😢

もう分かってると思いますが「最終要素まで見る」のメリットは、
違いがあった場合に確実に検出できることです。

最終要素まで見ることにしても、NUL終端規則が崩れることはないと思っています。

• テストコードでは、NUL終端規則を無視したデータをいれました。
  これは意図してやってることでしたが、テストコードなら許されると考えています。
• wcsncmp(a, b, _countof(a)) としてもNUL終端規則の無視にはならないと思います。
  これは「アプリ内での整合性を保つべき」にも矛盾してないと思います。

[berryzplus]

そうですね、Javaを見習ってなるべくグローバル変数と関数（namespace内に限定しようがNG）を使わない縛りプレイをするのも良いかもしれないですね。

Windows API プログラミングでは、WindowsとMSVCRTの設計に引きずられて、
グローバル関数やフリー関数を使わざるを得ないので、両方のスタイルでやれるのがベターだと思っています。
需要がなさそうなので話題にしてませんが、拡張子 .c のソースコードもぼく的には許容です。

OOP原理主義者はシングルトンやstatic methodも使わないという話を読んでネタかな…と思いましたが。

java系専門用語に「staticおじさん」というのがあります。

プログラミング技術はあるのにオブジェクト思考できなくて
巨大なUtilクラス(オールstaticメソッド)を作っちゃう人のことです。

オブジェクト思考技術は設計畑のスキルですが、
未熟な設計者が起こした設計を熟練プログラマが実装すると
そういう状況が発生しがちのようです。

だから「おじさん」なのよね。

まー、クラス設計の設計書無視してUtilクラス作っちゃう爺さまをdisる意味で使われるケースもたまにあるみたいです。

比較する左辺と右辺のうち片方もしくは両方がNUL終端し忘れた場合に、等値比較が最終要素を見るようにする事で解決される問題は、「同一のバイト列かどうかの判定が厳密になる」だけじゃないでしょうか？

そうです。比較演算子の判定結果が信頼できるものになるってだけのメリットです。

[berryzplus]

横暴にも「受け入れ不可と判断しました」と宣言して、それに対して「了解しました」の旨のコメントが付いた状況で、PRを放置しておくのもおかしな感じなので、closeにしてしまいます。

[beru]

テストで仕様を定義するというよりは、テストにより定まる仕様がある、という風に考えています。

テストが assert によって想定結果と実行結果を突き合わせることで、「～でなければいけない」「～であってはいけない」という形でプログラムの外形が決められてしまいます。それを仕様と呼んでいます。

テストが確認することが仕様のすべてではないし、仕様のすべてをテストに落とし込むべきだとも考えてはいませんが、少なくともテストが仕様の一部を定めます。これは意思の表明ではなく現実がそのように解釈されるということで、否定できないことです。

テストによって仕様が定まるから、テストは仕様に対して書きます。それを徹底することで、仕様を変えない実装の修正があってもテストはそのまま通用します。できていなければ、「PR #1093 等価比較演算子の実装がラクになる方法を探ってみる」で見られたように、何かするたびにテストの修正が同時に必要になります。テストが修正箇所を増やすだけの枷になります。

この理解が１つ前のコメントの末尾につながります。>「楽をするなら、邪魔になるテストを追加する手間も省いてもらわなくては困ります。」

私は徹底した怠け者です。(潜在バグ修正の名の下に)余計な手間を増やすだけの手間をかける行為(PR #1079)が許せなかったのです。

色々な条件で試験するテストコードを書く中で、実装当初は想定していなかったコーナーケースへの対処が必要な事が明確化して、書いたテストに合わせて実装を修正する、みたいな事を言ってるのかなと思いました。

ただテストコードで仕様が定まるといっても定型的に仕様を文書化しているわけじゃないのが気になります。プロトタイプ宣言や実装の近くにコメントでI/F仕様の説明が書かれている方が、そのI/Fを利用する上で使い方を理解するのに役立つような気がします。

まぁ実装もコメントもテストも、長い目で見て品質確保したいなら面倒でもちゃんと書きましょう、って事になりそうですが…。

[ds14050]

@berryzplus さんの方針がそれであるならそちらへ向かうのもいいでしょう。入出力コードを含めたすべての場所で、ヌル終端文字列を扱っているという前提を捨て去ったコードが書かれるべきだというわけです。

#903 (comment) でも書いたことですが、「自分の言葉に自分で従え」ということを言っているのです。諦めたのではありません。理解した上で馬鹿をやる自由を私は認めている、というだけのことです。最低限求めていることが「理解すること」です。

このデータ領域は、サクラエディタでない任意のプロセスから自由に書き込みが行える代物なので、NUL終端のローカルルールが守られる保証はありません。

したがって、利用者全員がNUL終端規則を守ることを前提に変更を許容することができなくなりました。

この PR を拒絶する理由として「このデータ領域は、サクラエディタでない任意のプロセスから自由に書き込みが行える代物なので、NUL終端のローカルルールが守られる保証はありません」を挙げています。これを前提にコード全体を再構成する考えであるなら、そうすればいい、と言っているんです。

できないし、やらないでしょう。そういう人間です。日本語も C++ も一貫性がなく矛盾していて、それを認めもしない。そういう人間です。

[ds14050]

現時点でお守りと尻拭いができるポジションにいるのは @beru さんですから、外野は健闘を祈るだけにします。くれぐれも、放り出すことがないように。

[berryzplus]

ええと。スルーで。

[beru]

MYDEVMODE 構造体の用途的に変数宣言時にゼロ初期化済ましておけば、同じかどうかの比較は構造体全体をmemcmpで十分ですよ。

いちいち個々のメンバーを比較する比較演算子をPOD型の構造体に追加したり、必ず存在する筈の終端文字の絡みで固定長配列の最終要素まで比較するかどうかを長々と議論したり、実アプリの改善に寄与しないテストコードを整備したりとか、みんないったいどうしちゃったんでしょうか…。サザエさんの中島に『お～い!磯野～!XPしようぜ!』とか言われたのかな。

[beru]

振り替えると #877 も #1079 も実質的に意味が無い変更だと思います。@ds14050 さんを召喚するオカルトチックな効能は確認出来ました。

[berryzplus]

対応内容をDLL_SHAREDATAに横展開するつもりがあります。

横展開対象はStaticStringを含む、DLL_SHAREDATAのメンバーとなり得る全てのPODクラスです。(DLL_SHAREDATA自体がPODクラスなので要するに配下全部です。)

対象が多いのもあって同基準での一括対応としたいので、詳細にこだわっています。