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

SonarQubeの静的解析で検出されたFileNameSepExtのバグを修正する #1796

Closed

Conversation

berryzplus
Copy link
Contributor

@berryzplus berryzplus commented Feb 12, 2022

PR の目的

SonarCloudにバグだと言われているコードを修正し、見た目の品質を上げます。

カテゴリ

  • リファクタリング

※検知されたのは「nullチェック漏れによるクラッシュの可能性」ですが、
 実態として実害はないと考えられるためリファクタリングとします。

PR の背景

SonarCloudにより以下の不具合が検出されています。
https://sonarcloud.io/project/issues?id=sakura-editor_sakura&issues=AXhxfE2dTNlLrH0R7vj9&open=AXhxfE2dTNlLrH0R7vj9

utif/file.cpp 内のローカル関数 FileNameSepExt の第一引数 filenullptr が渡された場合の考慮がないよ(≒渡ってきたら落ちるよ)という指摘です。

PR のメリット

  • SonarCloudにバグと検出される残念なコードが1つ消えます。

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

  • グローバル関数が1つ増えます。

仕様・動作説明

アプリの仕様・機能に変更はありません。

utif/file.cpp 内のローカル関数 FileNameSepExt のシグニチャを変更します。

  1. テストコードを書けるように static を外してグローバル関数にします。
  2. 第一引数 filenullptr の場合を考慮しなくていいように引数型を std::wstring_view に変更します。
  3. out引数のバッファサイズを受け取れるようにテンプレート関数にします。

PR の影響範囲

Diffダイアログに表示するファイル名比較に使われる関数です。

テスト内容

テスト1

手順

関連 issue, PR

参考資料

@sonarqubecloud
Copy link

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 2 Code Smells

100.0% 100.0% Coverage
0.0% 0.0% Duplication

@AppVeyorBot
Copy link

@berryzplus
Copy link
Contributor Author

berryzplus commented Feb 12, 2022

MinGWビルドがエラーになってるので修正します。

ただ、Code Smells の対応は呼出元に影響するので、対応を見送ろうと思っています。

Code Smell A 2 Code Smells

@berryzplus
Copy link
Contributor Author

なんだこれ?
MinGW Releaseだけ失敗している・・・。

@berryzplus
Copy link
Contributor Author

呼出元 FileMatchScoreSepExt を修正しないと対応が難しそうなので閉じます。

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.

2 participants