SonarQubeの静的解析で検出されたCFontAutoDeleterのバグを修正する #1799
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
CFontAutoDeleterは似非スマートポインタ。 コピーコンストラクタが未実装で、削除済みフォントを何度も削除しようとしてWindows GDIにダメージを与える危険があったのを修正する。
kengoide
reviewed
Feb 12, 2022
tests/unittests/test-window.cpp
Outdated
| */ | ||
| TEST( CFontAutoDeleter, test ) | ||
| { | ||
| CFontAutoDeleter deletor; |
There was a problem hiding this comment.
https://wikidiff.com/deleter/deletor
https://english.stackexchange.com/questions/60660/deleter-or-deletor
deleterが名詞で、deletorは動詞だそうです。
ここの文脈は名詞だからerのが良さそうです。
いまやってる対応が終わったら直しときます。
| #include <windowsx.h> | ||
| #include <Shlwapi.h> | ||
|
|
||
| #include <cstdlib> |
There was a problem hiding this comment.
不要なヘッダをインクルードしているように見えます。
- tchar.h
- shlwapi.h
- cstdlib
細かい点ですので修正してほしいという要求ではありません。
|
✅ Build sakura 1.0.4033 completed (commit 471565fe31 by @berryzplus) |
|
✅ Build sakura 1.0.4035 completed (commit 6b8b69ef91 by @berryzplus) |
|
これを入れたらvisual studioのIDEでSonarCloudに表示されるのとほぼ同じ警告が表示されるようになりました。 |
|
Kudos, SonarCloud Quality Gate passed! |
|
✅ Build sakura 1.0.4039 completed (commit 66f6f330e8 by @berryzplus) |
|
レビューありがとうございます。マージしてしまいます。 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
PR の目的
SonarCloudにバグだと言われているコードを修正し、見た目の品質を上げます。
カテゴリ
PR の背景
SonarCloudにより以下の不具合が検出されています。
https://sonarcloud.io/project/issues?id=sakura-editor_sakura&issues=AWnxcxv5O9B58BDk-CU-&open=AWnxcxv5O9B58BDk-CU-
utif/window.hで定義されているクラスCFontAutoDeleterのコピーコンストラクタ・コピー代入演算子が欠落しているよ（≒コピー時に何か追加の処理が要るとすると処理が漏れるよ）という指摘です。PR のメリット
PR のデメリット (トレードオフとかあれば)
仕様・動作説明
アプリの仕様・機能に変更はありません。
CFontAutoDeleterの潜在不具合を修正します。
CFontAutoDeleterは似非スマートポインタです。
スマートポインタなので、インスタンス破棄時にフォントオブジェクトを削除しています。
コピーコンストラクタが実装されていなかったため、
インスタンスをコピーした場合に削除済みフォントオブジェクトを再度削除しようとする危険がありました。
削除済みのフォントハンドルを
DeleteObjectした場合の結果は保証されないです。対策として、コピーコンストラクタとコピー代入演算子を実装して、問題が起きないようにします。
同時に、GetFontメソッドを追加してクラスの単体テストが書けるようにします。
同時に、まったく利用されていない無駄なメンバ変数を削除します。
PR の影響範囲
コンボボックスを表示する各種ダイアログで設定したフォントを保持するために使用する似非スマートポインタに関する変更です。
テスト内容
このPRは、修正部分のテストを含んでいます。
テスト1
手順
関連 issue, PR
参考資料