MYDEVMODEの等価比較演算子の隠れバグを修正する #1079
Conversation
|
❌ Build sakura 1.0.2349 failed (commit a78b14e0e0 by @berryzplus) |
|
✅ Build sakura 1.0.2350 completed (commit f43fa6858c by @berryzplus) |
|
つい最近読んだ本に「CLEANコード」という概念がありました。CLEANは頭字語でもあり、A は Assertive(高品質のコードは断定的だ)ということです。「まっすぐで狭いところを歩く(ものごとが意味をなさなくなった瞬間に、道を外れたことに気づけるように)」というアドバイスもありました(別の本でも見かけた文句の気がします)。 このテストは sz (Zero-terminated String) と名前の付いたメンバ変数にヌル文字が１つも存在しないケースを想定し、受け入れ、対処する必要性を示唆するように見受けられますが、その必要性は実在しているのでしょうか。自分は MYDEVMODE 固有の事情は知りませんが、(仮に存在するとして)対処すべきバグはヌル終端していない文字列をメンバとして保持するコードであり、テストは無駄であるばかりか、無用な示唆と疑念をコードを読む人間に与えるものではないかと考えています。 このコメントは @beru さんが将来似たケースで異なる判断を下すといいなあと期待して書いています(迷惑は承知のうえ。期待するのが勝手なら応える応えないも勝手ですので)。 |
|
レビューありがとうございます。 |
一応、strncpy的な関数の使い方を誤るとNUL終端されない文字列を作れますので、絶対要らない対応、というわけでもないと思ってました。 このPRは思い付きを出してみた感じのものなので、切迫感はあんまりなかったんですけど 😄 |
|
領域内の文字列データがきちんとNULL終端しているかのチェックを、デバッグビルドの場合に適切な箇所で自動で実行出来れば良いですね。文字列型を介したコードにすれば実現しやすそうですけど、でもちゃんと動いてる箇所を今更書き換えるのも自己満足になってしまうかな。。 C/C++言語はメモリ操作がしやすいですけど意図を外れてメモリ破壊する処理を簡単に書けてしまうので注意が必要ですね。 |
関心の分離や単一責任の原則に通じるところがあるんでしょうか？
文字列領域のメンバーの内容によってそれを保持する型の非等価演算子が正しい動きをするかのテストがこのPRで追加されたわけですが、同じような作りの型が他にたくさんある場合に、それらに対しても同じ内容のテストを追加するとなると骨が折れますね。 そう考えると固定長の領域上をNULL終端文字列として扱う型を用意し、その型のテストで挙動の妥当性を確認するように出来ると良いんでしょうか…。
自分はあんまりチェックせずに気軽に Approve してるので期待に添えるかは微妙ですね…。 |
しかしテストコードって書くの面倒くさい上にあんまり役立っている気がしないような…とか書いてしまうと他の人の活動にケチをつけているようで良くないですね。。こんな事を言っているとバグを入れた時に今後テスト書く事を強制されてしまう。。ﾌﾞﾙﾌﾞﾙ…。 |
|
あ、MYDEVMODE って class ではなくて struct だったんですね。うーん、坊主憎けりゃ袈裟まで憎いだったかなあ。 MYDEVMODE の比較演算子が null 終端されていない文字列を想定し、区別すべきであるとテストコードによって規定するのが、お門違いでなんの足しにもなっていないのは間違いないと考えますが。 |
このままクローズした PR に愚痴られても面倒なので、とりあえず思ったこと書いときます。 何勘違いしてんだこのアフォは。 ぼくは @Kenchi さんのようにオトナじゃないです。 「本物は違うぜ！」ってところは見飽きました。 以上。 |
|
きっかけは「(潜在)バグ認定」です。 #1010 (comment) には全面的に同意しますが、この PR は違います。NULL 文字が入っているべきとき、ところに、それ以外の文字を許容し、有効文字列長を１文字分だけ伸ばすことがあるべき動作だとは思いません。バグってるのはどっちなんだって話です。百歩譲ってどちらもアリだとしても、やっているのはバグ修正ではなく仕様変更です。バグったコードは存在していなかった。 他にもことあるごとにサクラエディタのコードベースがうんこだと吹聴するのを不快に思っています。 |
C++ の class と struct の差異はメンバーのアクセス修飾子がデフォルトで public か private かですけれど、
まぁでも MYDEVMODE のインスタンスって領域の初期化さえちゃんとやってれば memcmp で比較しても良いんじゃないかなぁ。。たくさんあるメンバーの１つ１つを突き合わせる記述を書くのとか億劫なので。。まぁ一度書いてしまってメンバー追加しないならそれ以上労力が掛からないだろうから書く手間を惜しむのも良くないかもしれないですけど。。まぁこんな考えでいるとバグに苦しめられるかな。。。 コードやその書き手に関しては、リアルのプロジェクトでコンスタントに実害を受け続けてるとかじゃない限り、そこまで敵視しないでも良いんじゃないかなぁと思いました。 |
強いなぁ、相変わらず・・・そういうとこ好きですが。 バグ認定した対象は、ぼくが書いたコード です。 サクラエディタの設計が一部テキトーな感じになっているのは事実です。 「うんこコード」という言い方が不快であるなら、それはすまんです。 |
memset で初期化する前提付きでそれは否定しません。その結果 null 文字(※あるかもしれないしないかもしれない)の後ろのゴミデータの影響を受けるようになるよ、というのもコストとパフォーマンスを天秤にかけられます。 この PR に天秤にかけられるパフォーマンスがあるのか、ということです。
優しいですね。自分はそれに甘えていますが、それも終わりにした方が良さそうです。 |
やっぱり勘違いしてると思いますよ。 このPRがやってることは、 通常buffer[4]にはNULが入ります。 後ろのゴミデータの影響を受ける余地はありません。 |
|
性分ですので訂正せずにはいられません。
memcmp で実装するなら(※これが以前の実装です)、パディングやヌル文字以降のゴミデータの影響を避けるために初期化や、メンバの上書きに制約が付きますが、実装は簡単簡潔です。MYDEVMODE はそれを許す C でおなじみの POD 構造体です。 この PR がやっていることは、手間をかけて紛らわしい仕様をテストにより規定する行為です。簡単な実装を選んだがゆえに取り扱いに注意を要するという以前とは異なるが、かといってヌル末端文字列に対する当たり前の期待に応えるようにもなっていません。手をかけて何をやっているんだというのが「Confused」アイコンの意味です。 |
|
ACK がないので念のためにもう一度書きます。これは自分の性分であり自分の満足のために行う、国語の時間です。
第一段落と第二段落は対比関係にあります。第一段落が memcmp を使った比較について、第二がこの PR による実装について。 memcmp による構造体の比較がヌル文字の存在に頓着せず、ゴミデータの影響さえも受けるのは実装の簡便さを優先したがゆえの制約です。 memcmp の使用をやめてからはヌル文字を要求し、ゴミデータの影響を受けなくなりました。 この PR はヌル文字の要求を放棄しました。ヌル文字を保証しながらテストに期待される比較演算結果を得ようとすると、バッファが１ |
|
具体的に何が困るのか分からんです。 もしかしてこういうケースを想定してます？
なんとなく、これとは違うことを指摘しようとしてる気がしてます。 |
|
成立しない理解と対話に一方的に消耗するのが業腹だと今まで未読スルーしていました。すみません。指摘すべき点があります。
「バッファの末尾の一文字を比較対象にして区別できなければいけないとテストにより規定すること(※この PR に対する自分の理解)」はヌル終端を保証するコードを書くことを妨げ、ヌル終端されていることを期待したコードを潜在的にバグを抱えたコードへと変貌させます。 |
|
ああ、すんません、 ¥nは¥0の誤記なのは指摘の通りです。 |
|
で、それはそれとして。 何が問題だと言ってるか、読み取れませんでした。 |
これ本当？ |
を実行したところ 0 が出力されました。 |
|
|
たしかに wcsncmp の仕様の詳細は「それはそれ」です。 すでに書いた通り、「「バッファの末尾の一文字を比較対象にして区別できなければいけないとテストにより規定すること(※この PR に対する自分の理解)」はヌル終端を保証するコードを書くことを妨げ、ヌル終端されていることを期待したコードを潜在的にバグを抱えたコードへと変貌させます。」がすべてです。 この PR をマージする前に sz メンバが利用されているすべての場所を調べ、ヌル文字の存在を前提としたコードが書かれていないかを確かめ、書かれていたならヌル文字が存在しなくても問題がないようにプログラムを修正する準備があったでしょうか。 ヌル終端があるのかないのか、ヌル終端があると期待して良いのかダメなのか、はっきりさせなければバグと無駄のないコードは書けません。Assertive なコードが求められる所以です。テストコードから判断する限りヌル終端は保証されません。テストを満足させながらヌル終端を必ず付けることはできません。ヌル終端のあるなしで構造体が異なるものと判断されるために勝手にヌル終端を付けることは構造体利用者の意図しないデータの改変になります。テストによりヌル終端がない場合があるとはっきり示され、どのように振る舞うべきかが定義されているために、構造体でデータを交換するものの間でたとえそれが暗黙的なものであってもヌル終端を期待することが難しくなっています。それがコードから読み取れることです。ある瞬間に大臣の職にあっただけの者が「新法をそのように悪用するつもりはない」と言明したところで、後世は法に従うのみで法に書かれていることがすべてです。コードが法です。 |
当初の実装は wmemcmp を使っていてヌル文字の後ろも、ついでにいえば構造体のパディングも比較対象にしていたようですが、それはあるべき仕様というよりは実装の簡便さをとった結果だと理解しています。 なので wmemcmp を廃して wcsncmp を使うようにした「関連チケット #877」は前進であり、今また wmemcmp を使うとすればそれは後退だと思います。 指摘は事実ですがそれを望んでいる者はいないのではないでしょうか。 |
|
#1079 (comment) thanks! 自分でもやってみて、wcsncmpの挙動に誤解があったことを把握しました。 やってみたテスト
で、指摘のポイントがよくわかってないんですけど、結局 #1093 |
#1093 は内容を見る前にこちらの PR への言及から飛んできてしまいました。 そちらの PR でテストが失敗しているのはこの PR が追加したテストケースが認められないもので不適切だからですよね。それがこの PR に疑問符を付けた理由です。 ヌル終端が存在するという当たり前の期待ができなくなる、ヌル終端を保証するコードを書くとテストが通らなくなる、「正しい」コードが書けなくなる、という理由です。 ヌル終端が存在しないケースを扱えるということも「間違った」コードではありませんが当たり前ではないので、構造体のドキュメントに特記が必要になると思いますし、Windows がそういう値をよこしてくるという必要性に迫られているのでないかぎり無用だと思います。 |
|
#1093 の話題ですが分散しないようにこちらへ。string_view を試してみました。以下のようなコードです。 出力が下の通り。 wcsncmp を使っていたときにはあった、ヌル終端されていないバッドケースとバッドケースを比較した際に暴走しないための保険が、string_view を使うと失われます。 しかしそういう保険を積極的に掛けようとしているのは他ならぬ @berryzplus さんなので、自分で OK を出すなら反対する人はいないかもしれないですね。 |
|
パラノイア気味かとも思いますが、エッジケースにおいてヌル文字が書き込まれたり書き込まれなかったりする文字列関数を呼び出した後、自分でヌル文字をバッファの最後に書き込むことがあります。 ドキュメントを正しく理解できているか、ランタイムによる差異がないか、ランタイムのバージョンによる差異がないか、色々考えるより自分で保証する方が安心確実だからです。 string_view を利用する場合でも、事前に無条件にバッファの末尾にヌル文字を書き込んでおくことで wcsnlen と assert を組み合わせるのとは違った形の安全策になります。それをするかしないかは「パラノイア」の度合い次第ですが。 このような安全策をとる可能性を潰すから、この PR が追加するテストケースとそれに合わせた実装の変更に疑問符を付けたのです。 |
|
いったん整理。 関連チケット#877 構造体比較にmemcmpを使うのをやめる 修正前の模擬コードstruct AAA {
wchar_t aa[3];
wchar_t bb[3];
bool operator == (const AAA& rhs) const {
return 0 == wmemcmp(aa, rhs, _countof(AAA));
}
};
AAA aaa{ {L"aa"}, {L"bb"} };
wchar_t* szBbb = L"aa\0bb\0";
AAA& bbb = *reinterpret_cast<AAA*>(szBbb);
ASSERT_EQ(aaa, bbb);
#877が解決した問題struct AAA {
wchar_t aa[3];
wchar_t bb[3];
bool operator == (const AAA& rhs) const {
return 0 == wcsncmp(aa, rhs.aa, _countof(aa) - 1)
&& 0 == wcsncmp(bb, rhs.bb, _countof(bb) - 1);
}
};
AAA aaa{ {L"aa"}, {L"bb"} };
wchar_t* szBbb = L"aa\0bb\0";
AAA& bbb = *reinterpret_cast<AAA*>(szBbb);
ASSERT_EQ(aaa, bbb);
★1 のケースが「必要か？」というと別に要らんと思っています。 #1079が解決した問題struct AAA {
wchar_t aa[3];
wchar_t bb[3];
bool operator == (const AAA& rhs) const {
return 0 == wcsncmp(aa, rhs.aa, _countof(aa))
&& 0 == wcsncmp(bb, rhs.bb, _countof(bb));
}
};
AAA aaa{ {L"aa"}, {L"bb"} };
wchar_t* szBbb = L"aa\0bb\0";
AAA& bbb = *reinterpret_cast<AAA*>(szBbb);
ASSERT_EQ(aaa, bbb);
wchar_t* szCcc = L"aaabb\0";
AAA& ccc = *reinterpret_cast<AAA*>(szCcc);
ASSERT_NE(aaa, ccc);
#1093 の提案内容struct AAA {
wchar_t aa[3];
wchar_t bb[3];
bool operator == (const AAA& rhs) const {
return std::wstring_view(aa) == rhs.aa
&& std::wstring_view(bb) == rhs.bb;
}
bool operator != (const AAA& rhs) const {
return !(*this == rhs);
}
};
AAA aaa{ {L"aa"}, {L"bb"} };
wchar_t* szBbb = L"aa\0bb\0";
AAA& bbb = *reinterpret_cast<AAA*>(szBbb);
ASSERT_EQ(aaa, bbb);
wchar_t* szCcc = L"aaabb\0";
AAA& ccc = *reinterpret_cast<AAA*>(szCcc);
ASSERT_NE(aaa, ccc);
#1093 の提案内容を一歩進めてみるstruct AAA {
wchar_t aa[3];
wchar_t bb[3];
bool operator == (const AAA& rhs) const {
return 0 == std::wstring_view(aa).compare(rhs.aa)
&& 0 == std::wstring_view(bb).compare(rhs.bb);
}
bool operator != (const AAA& rhs) const {
return !(*this == rhs);
}
};
AAA aaa{ {L"aa"}, {L"bb"} };
wchar_t* szBbb = L"aa\0bb\0";
AAA& bbb = *reinterpret_cast<AAA*>(szBbb);
ASSERT_EQ(aaa, bbb);
wchar_t* szCcc = L"aaabb\0";
AAA& ccc = *reinterpret_cast<AAA*>(szCcc);
ASSERT_NE(aaa, ccc);
なお、 |
|
「#877 構造体比較にmemcmpを使うのをやめる」の時点ですでに悪しき仕様がテストに織り込まれていたんですね。気がついていませんでした。 例外条件で落ちないことを確認するのに ASSERT はいりません。悪条件を用意した後で演算子を呼び出すだけでいいんです。ただし結果は捨てます。特定の結果を要求すると、例外条件が正常ケースになります。そこから無理が生じています。 |
気付いてなかったならしょうがない・・・ 何をしても落ちないを「悪しき仕様」と言われてる気がしてなんか引っかかりました。 MYDEVMODEの等価比較演算子には なお、変数名のプレフィックスszの意味は、 分かってないやつのが多い と思います。 |
|
@berryzplus さんの意見が 日本語で 聞きたいです。MYDEVMODE の文字列メンバのヌル終端に関する仕様です。実装とテストを用意するにあたり、どういう仕様を頭の中で定めているのかです。べき論や理想ではなく、現実としてどういう仕様を想定して、実装とテストを用意しているかです。 余談。 入力次第で例外を投げるかもしれない関数に noexcept を付けるなら noexcept は飾りです。もしくは C++ の言語仕様としての例外処理が飾りになります。 sz の意味がわかっている人は、sz 接頭辞がついたメンバにヌル文字の存在を期待します。ヌル文字が存在しない場合にどういう比較結果を返すべきかは不明です。穏当な解決策は、実際に書き込むか書き込まないかはオプションですが、ヌル文字が存在しているかのように比較することです。ヌル文字が存在しない場合を許容し、バッファの全長に渡り辞書式の比較結果を返すことを約束するなら、sz 接頭辞は嘘です。 能書きを過不足なくコードで表現することが肝要です。しかし母語が不自由な人間は、プログラミング言語を操ることにも不自由するでしょう。 |
|
以下の通り理解しました。
それでは済まされないと考えてしまう理由がこれです。
テストが存在し、ASSERT により求められる結果が定義された時点で、「どっちでもいい」がどっちでも良くなくなっているんです。そしてその内容が良くないものだから、テストがない方がマシなのです。 しかしこんなのはきっかけの部分に勘違いがあったが故です。他人が差し出口をしました。 |
現実として、MYDEVMODE は C言語 の構造体なので、各メンバ変数の名前は、メンバの識別以外の意味を持たないと考えています。当然 sz プレフィックスは嘘です。外部から容易にアクセス可能な、ただの連続データの一部領域に書き込むデータに制約をかける方法はありません。 演算子の実装仕様(文字列メンバの比較について)
役割は「あくまで比較」で「妥当性の検証」ではないので、問題ないと思います。 内容の8割がプログラミング専門用語で構成される文章を日本語扱いしていいのか？ みたいな倫理的課題は別にして。 演算子のテストの実装仕様
キミがそれを言うなよ（核爆 と返されることを織り込んだうえでギャグ書いてるのか、 このやりとりは、キミがぼくに対して「何が問題なのか」を正しく伝えられないがために続いていると思っています。「日本語がダメならコードで書いてくれてもいいんだよ」というのは、ぼくも言ってることで、それすらできないから話が終わんないんだと思います。 |
|
コードにより保証できないから、テストにより、ヌル終端があるという暗黙の前提を許さないということですね。たしかにこの PR に含まれる比較演算実装の修正は、それに沿うものです。 テストにより規定されるこの新しい状況では、比較演算実装に wcscmp(n がないバージョン)や string_view を使うことが許されません。アクセス違反が避けられないからです。 ではどうして「wcscmpではなく wcsncmpを使う方が安全 という指摘があったので変更した。 ★1 wcsncmp を使わなかった場合に何が起きるかを示すため StrategyForSegmentationFault を追加した。 ★1 のケースが「必要か？」というと別に要らんと思っています。」というような矛盾した言動が出てくるのでしょうか。 どうして「#1093 等価比較演算子の実装がラクになる方法を探ってみる」のような string_view を使う PR を後から出してきて、テストを撤回しようとするのでしょうか。 何をやっているのでしょうか。自分がついさっきまでどちらを向いて進んでいたかも理解できていないから、右往左往するのです。 |
ぼくが人間だからです。
ぼくが人間だからです。
最善策の模索です。 やってみないと分からないことはあるし、 右往左往してるのをあえてみせたことで、 |
|
開き直りとはいえはじめて認めましたね。よくできました。 |
…_equality_of_MYDEVMODE MYDEVMODEの等価比較演算子の隠れバグを修正する
PR の目的
MYDEVMODEの等価比較演算子の実装に潜在バグを見付けたので修正します。
カテゴリ
PR の背景
他のクラスのテストを書くために、
等価比較演算子をコピペ実装していく作業の中で気付きました。
具体的に何がマズいのか説明しづらいので、少し変わったやり方で進めたいです。
テストが失敗するので、何が問題なのか共有できます。
テストが成功するので、直ったような気分になれます。
MYDEVMODE自体はあまり重要なクラスでもないので、直さないとダメか？っていうとそうでもないんですが・・・。
PR のメリット
PR のデメリット (トレードオフとかあれば)
PR の影響範囲
関連チケット
#877 構造体比較にmemcmpを使うのをやめる
参考資料
とくにありません。