Skip to content

Conversation

@berryzplus
Copy link
Contributor

@berryzplus berryzplus commented Jun 6, 2021

PR の目的

SonarCloudの警告を減らすことが目的です。
コード妥当性と存在意義の怪しいクラスを削除します。

カテゴリ

  • リファクタリング

PR の背景

SonarCloud解析で以下のようなBugs警告(Criticalレベル)が検出されています。(#1504)

  • Explicitly define or delete the missing destructor and copy assignment operator so that they will not be implicitly provided.

    CharPointerA 既に未使用なので削除してしまいます。

  • Explicitly define or delete the missing destructor and copy assignment operator so that they will not be implicitly provided.

    CharPointerW まだ利用されてるので対策が必要です。
    警告の超訳:コピーコンストラクタが定義されていますが、デストラクタとコピー代入演算子が未定義です。

警告自体は、これまで対応してきた「rule of five」のパターンで対処できます。

しかし、CharPointerW には複数の問題があることが分かりました。

const wchar_t* operator ++ () { _forward(); return this->m_p; } //!< ++p;
const wchar_t* operator ++ (int){ CharPointerW tmp; _forward(); return tmp.m_p; } //!< p++;
const wchar_t* operator += (size_t n){ while(n-->0)_forward(); return this->m_p; } //!< p+=n;

  • 前置インクリメント演算子のコメントが「コメントアウトされたコード」になっています。
  • 後置インクリメント演算子のコメントが「コメントアウトされたコード」になっています。
  • 後置インクリメント演算子の実装がバグっています。本来の後置インクリメントは、インクリメント前のインスタンスをコピーしておき、自身をインクリメントしたのちにコピーした値を返します。👆のコードではコピーするのを忘れているので正しい値が返りません。
  • 加算代入演算子のコメントが「コメントアウトされたコード」になっています。
  • 加算代入演算子の実装が不適切です。(注意:バグではないです。)
    やりたいこと(≒ただのポインタ加算)に対してコードが複雑すぎます。

これらの問題に対処するのは不可能と思うので、ファイルごと削除してしまいます。

PR のメリット

  • SonarCloudで検出されるBugs(Critical)レベルの警告が少し減ります。

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

  • とくにありません。
    警告が検出された部分をファイルごと削除してしまうので、警告が減らない可能性はないと思います。

仕様・動作説明

アプリの機能に影響を与えるような変更ではありません。

このPRでは、問題が検出されたクラスに依存するコードを書き替えてクラス自体を削除します。

PR の影響範囲

CharPointerWを利用していた関数を使う処理に影響します。
変更前後で関数の挙動を変えないため、実害はないと考えています。

テスト内容

CharPointerWに依存するグローバル関数2件についてテストコードを作成し、変更前後で動作が変わらないことを確認しました。

関連 issue, PR

#1504
#1605

参考資料

@berryzplus berryzplus force-pushed the feature/remove_CharPointerW branch from 96ddb51 to b47ccff Compare June 6, 2021 15:43
@AppVeyorBot
Copy link

@sonarqubecloud
Copy link

sonarqubecloud bot commented Jun 6, 2021

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

100.0% 100.0% Coverage
0.0% 0.0% Duplication

@AppVeyorBot
Copy link

@beru beru added the refactoring リファクタリング 【ChangeLog除外】 label Jun 7, 2021
@berryzplus berryzplus changed the title SonarCloudで検出された問題のあるクラスCharPointerWを削除する CharPointerWを削除する Jun 12, 2021
@berryzplus berryzplus marked this pull request as ready for review June 12, 2021 07:29
// ドライブ名はパスの深さに数えない
if( ((L'A' <= path[0] && path[0] <= L'Z') || (L'a' <= path[0] && path[0] <= L'z'))
&& path[1] == L':' && path[2] == L'\\' ){
if (std::regex_search(path, std::wregex(LR"(^[A-Z]:\\)", std::wregex::icase))) {
Copy link
Contributor

Choose a reason for hiding this comment

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

この変更はPRの説明には書かれている内容とは違いますが必要な変更でしょうか?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

いいえ、変更しなくても目的は達成できます。

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

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

@berryzplus berryzplus merged commit c1568de into sakura-editor:master Jun 14, 2021
@berryzplus berryzplus deleted the feature/remove_CharPointerW branch June 14, 2021 15:51
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