構造体比較にmemcmpを使うのをやめる

[berryzplus]

目的

SonarQube導入PRのレビューで実験的に行った修正を適用します。

SonarQubeの指摘内容

https://sonarcloud.io/project/issues?id=sakura-editor_sakura&open=AWnxcxsuO9B58BDk-CUh&resolved=false&types=BUG

修正内容

構造体の比較でmemcmpを使うと、構造体内のパディング部分の差異で等価判断を正しく行えなくなることがあります。構造体に比較演算子を実装して、比較演算が正しく行われるようにします。

このPRに関しての備考

同種の指摘を全部つぶしにいこう、という意図ではありません。
せっかく修正したしもったいないから入れとこう程度の認識です。

[berryzplus]

期待した感じにリンクが作れなかったので貼り直し。
https://sonarcloud.io/code?id=sakura-editor_sakura&selected=sakura-editor_sakura%3Asakura_core%2Fprint%2FCPrintPreview.cpp
1077行目。

GitHub

[m-tmatma]

あと、単体テストの実装お願いできますか?

[berryzplus]

単体テスト。書いてみたのはいいんだけど、CodeFactorが警告を発している・・・。

等価演算子/否定の等価演算子のテストなので、
構造体の初期値にテキトーな値を突っ込んでいる。

数値型のテキトーな値といえばmin/maxなわけで、
これならば構造体メンバ定義をコピってきたものをテキスト整形すれば出来上がるわけで、
テストコードもそういう作り方をしたわけだが、それがDuplicateと判定されているわけで。
いまとっても Ignore したい衝動に駆られているわけで・・・:cry:

[berryzplus]

いまとっても Ignore したい衝動に駆られているわけで・・・cry

Ignoreしました。

[m-tmatma]

単体テスト。書いてみたのはいいんだけど、CodeFactorが警告を発している・・・。

何をするテストかのコメントがあってわかりやすいです。

• m_szPrinterDriverName 等の文字列で NULL 文字がないぐらいデータが詰まっていた時に
  落ちないことと、誤動作しないことの確認があるほうがいいと思います。
• 上限と下限だけでなく、真ん中らへんのテストもあったほうがいいと思います。(これはこのケースの場合そんなに必要ではないですが)

[berryzplus]

ふむ。Release版の単体テストが失敗しておる・・・。

•m_szPrinterDriverName 等の文字列で NULL 文字がないぐらいデータが詰まっていた時に
落ちないことと、誤動作しないことの確認があるほうがいいと思います。

この条件を証明するために、アクセスしてよいメモリ領域とアクセスしてはならないメモリ領域が連続して配置されるように api でゴリゴリして、「アクセスしてはならないメモリ領域にアクセスしました！」という例外（＝一般保護違反、セグメンテーションフォールト）を意図的に発生させるようなテストを書いた。

2019-04-30T14:45:11.7427924Z [ RUN      ] MYDEVMODETest.StrategyForSegmentationFault
2019-04-30T14:45:11.7431347Z D:\a\1\s\tests\unittests\test-mydevmode.cpp(303): error: Death test: { ::wcscmp(pValues[0].m_szPrinterDeviceName, pLargeStr); }
2019-04-30T14:45:11.7434257Z     Result: failed to die.
2019-04-30T14:45:11.7437127Z  Error msg:
2019-04-30T14:45:11.7440254Z [  DEATH   ] 
2019-04-30T14:45:11.7443405Z [  FAILED  ] MYDEVMODETest.StrategyForSegmentationFault (146 ms)

エラーメッセージは、死亡想定のアクセス違反コードでセグフォールトが発生しなかった、と取れる内容。とりあえず、このままはマズい・・・。

[berryzplus]

•上限と下限だけでなく、真ん中らへんのテストもあったほうがいいと思います。
(これはこのケースの場合そんなに必要ではないですが)

分かってそうだったのであえてスルーしました。
等価比較演算子の良し悪しを判定するのに必要なのは、異なる2つの値です。
上限下限の組み合わせでなくても、0と1とか、255と1024とかでもいいです。
範囲外でない限り、値域に意味はありません。

状況的に中間値付近のテストがどうしても必要と考えているように見えてるんですけど、やっぱり欲しいですか？

[m-tmatma]

状況的に中間値付近のテストがどうしても必要と考えているように見えてるんですけど、やっぱり欲しいですか？

いえ、不要です。

[m-tmatma]

エラーメッセージは、死亡想定のアクセス違反コードでセグフォールトが発生しなかった、と取れる内容。とりあえず、このままはマズい・・・。

時間がかかりそうなら、チケットだけ作っておいて後回しでもいいです。
そこは臨機応変に。

[berryzplus]

エラーメッセージは、死亡想定のアクセス違反コードでセグフォールトが発生しなかった、と取れる内容。とりあえず、このままはマズい・・・。

時間がかかりそうなら、チケットだけ作っておいて後回しでもいいです。
そこは臨機応変に。

対処は完了しています。 ⇒ c0d02c5 (MSVCリリース版ではテストを実行しない

チェック結果が × になってるのはCodeFactorのせいです。
エラー内容を確認しにいくと、No issue Found(1 ignored)となっています。

[berryzplus]

レビューありがとうございます。
マージしちゃいます。