Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CDllImp::DeinitDllをnoexceptとマークする #1857

Merged

Conversation

sanomari
Copy link
Contributor

PR の目的

Sonar Cloudの無駄な警告を潰して、ツールが検出した「本当に問題のあるコード」を識別しやすくします。

カテゴリ

  • リファクタリング

PR の背景

ここのprojectではコードの静的解析に Sonar Cloud を使っています。
検出されたバグを修正する試みが何度か行われてきました。
最近はやっていないようですが、現在 113 件のバグが放置されています。
https://sonarcloud.io/project/overview?id=sakura-editor_sakura

今回はこのうちの1つを対応したいと思います。
https://sonarcloud.io/project/issues?issues=AX-cH3QqiOmgBqQJ-eXH&open=AX-cH3QqiOmgBqQJ-eXH&id=sakura-editor_sakura

PR のメリット

PR のデメリット (トレードオフとかあれば)

仕様・動作説明

対象のコードは以下です。

CDllImp::~CDllImp()
{
	if( IsAvailable() ){
		DeinitDll(true);
	}
}

DeinitDllが例外を投げた場合に発生する問題についての警告です。
「例外を投げるかもしれないメソッド」をデストラクタから呼び出しています。

対策としてDeinitDllを「例外を投げないメソッド(noexcept)」とマークします。
修正により、上記警告が消えます。

noexceptを付けるデメリットは、「もし例外を投げたらクラッシュすること」です。
stringのメモリ拡張コードとか書けなくなります。
コードを見た感じ、そうなっても誰も困らなそうなので提案を出してみる次第です。

PR の影響範囲

CDllImp派生クラスのインスタンスを破棄する処理に影響する修正です。
アプリ終了時のタイミングにあたり、何か問題が起きたとしても普通は気付きません。

テスト内容

Visual Studioの出力ペインを睨みつけながら、
サクラエディタを起動して終了します。
出力ペインには何も出力されていないので致命的な問題は発生していないと思います。

関連 issue, PR

参考資料

@AppVeyorBot
Copy link

Build sakura 1.0.4159 completed (commit 4bb321c309 by @sanomari)

@sonarcloud
Copy link

sonarcloud bot commented Jun 14, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@sanomari sanomari merged commit 75e1a56 into sakura-editor:master Jun 16, 2022
@sanomari sanomari deleted the feature/fix_bad_destructor_of_CDllImp branch June 16, 2022 13:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants