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

SonarCloudにBugだと言われているCOutlinePythonの初期化漏れに対処する #1602

Merged

Conversation

berryzplus
Copy link
Contributor

PR の目的

SonarScanで検出されたBugsレベル警告に対処します。

カテゴリ

  • リファクタリング

PR の背景

SonarScanで以下のようなBugsレベル警告が検出されています。

PR のメリット

SonarCloudのBugsレベル警告が減少します。

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

アウトライン解析に関するコードであるため、単体テストを書けません。
意図しない不具合を作り込んでしまっている可能性があります。

仕様・動作説明

  • コンストラクターの member-initializer-listでm_quote_charを書き洩らしていることが警告の原因です。
  • SonarCloud解析ではコンストラクタ・デストラクタの定義を2つの基準で見ているようです。
    • Rule-of-Zero(処理が空なら何も書くな)
      必要がなければ独自のコンストラクタ・デストラクタを実装すべきではない。
    • Rule-of-Five(処理があるなら全部書け)
      必要があって独自のコンストラクタ・デストラクタを実装するときは5種類セットで書くべきだ。
  • コンストラクタは元々何もしてないのでm_quote_charだけ直すと別な警告(Rule-of-Zeroに従え)が発生すると予想されます。対策として in-class-initialize 方式に改めます。
  • メンバ変数 m_quote_char の型を誤解がないように改めます。
    • 変更前の定義型は int でした。
    • 変更後は wchar_t に改めます。
    • wchar_t は uint16_t 相当なので int(=int32_t) のままでも問題はありませんが、wchar_tでは表現できない文字コード値に対応しているかのような誤解を生むと思うので値域の狭い型に訂正します。

PR の影響範囲

Pythonのアウトライン解析に影響する修正です。

テスト内容

ビルド確認のみです。

関連 issue, PR

#1504

参考資料

@beru beru added the refactoring リファクタリング 【ChangeLog除外】 label Mar 21, 2021
Copy link
Contributor

@beru beru left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

この変更をするなら COutlinePython::ScanString において

int quote_char = m_quote_char;

の代わりに

auto quote_char = m_quote_char;

とするのはどうでしょうか? COutlinePython::m_quote_char の型が変更されたのでそれに追従する変更です。

@AppVeyorBot
Copy link

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!

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

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@berryzplus berryzplus marked this pull request as ready for review March 21, 2021 12:58
@AppVeyorBot
Copy link

Copy link
Contributor

@beru beru left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

問題無いと思います。

動作確認は行っておらずコードの変更内容だけで判断しました。

@berryzplus
Copy link
Contributor Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactoring リファクタリング 【ChangeLog除外】
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants