Skip to content

Conversation

@berryzplus
Copy link
Contributor

PR の目的

SonarCloudの警告を減らし、メンテナンスしやすいアプリに近づけることが目的です。

カテゴリ

  • リファクタリング

PR の背景

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

Explicitly define the missing destructor and copy constructor so that they will not be implicitly provided.
https://sonarcloud.io/project/issues?id=sakura-editor_sakura&issues=AWnxcyPsO9B58BDk-C3r&open=AWnxcyPsO9B58BDk-C3r

超訳:コピー演算子を定義するときは、デストラクタ、コピーコンストラクタ、ムーブコンストラクタ、ムーブ演算子がコンパイラにより意図しない形で自動生成されることを防ぐため、これらを明示的に定義してください。

問題のあるコードはこれです。

//代入
const CEol& operator=( const CEol& t ){ m_eEolType = t.m_eEolType; return *this; }

単純な対処としては、削ってしまうか、他4種類のコンストラクタ・デストラクタ・演算子を追加してあげれば良いです。

CEol型はサクラエディタの中核を成すデータ型です。

  • テキストエディタにとって、データは「文字列」です。
  • テキストエディタのデータは「行」という論理単位で小分けにされます。
  • 「行」とは、「行終端子」で区切られた「文字列」です。
  • CEolはエディタ内で「適切な行終端子」を保持するための型です。

そこで、今回は SonarCloud 指摘を「キッカケ」と考えて、
「行終端子」を扱うコードを全体的にリファクタリングしてみました。

PR のメリット

  • 「行終端子」の扱いに関する型定義が明確になり、結果として分かりやすくなります。
  • CEolクラスの主要機能に対し、単体テストが追加されます。
  • EEolTypeの定数名が「キャラクタ名略称」から「名前」になり、やや分かりやすくなります。
  • SonarCloudのBugs(Critical)レベル警告が1つ減ります。

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

  • EEolTypeの定数名を一括置換した影響でテストできない領域のコードを修正しています。
  • EEolTypeの定数名が長くなります。
  • enum EEolTypeenum class 化する影響で、修正範囲が大変広いです。

仕様・動作説明

説明されても分からんと思うので、ごく簡単にポイントだけ書きます。

  • EEolType型 「行終端子」を表す型です。
    定義には無効な値を含んでいます。

  • CEol型 行末記号の種類を保持する型です。
    EEolType型の定数を使って行末記号を保持します。
    保持できる値は、有効な「行終端子」または「行終端子なし」のみです。

  • Rule-of-Five 👈 SonarCloud指摘はこれ。
    必要があるなら全部書けルール

  • Rule-of-Zero 👈 PRの対処内容はこれ。
    必要がないなら書くなルール

  • C++11 を利用できる場合、列挙型には enum class を使うべきらしいです。

    • enum 型  EOL_NONEとか書きます。int型と暗黙変換できます。C言語互換です。
    • enum class 型 EEolType::noneとか書きます。暗黙変換できません。explicitが使えます。

PR の影響範囲

  • バイナリシーケンスないし文字列内に含まれる「行終端子」の検出・設定に関わる処理に影響する変更です。
    • ファイルを開くときの改行コード検出に影響します。
    • 「名前を付けて保存」の改行コード指定に影響します。
    • 「行単位」でデータを扱う処理に影響します。

テスト内容

テキトーなファイルを開いて「名前を付けて保存」してみてください。
「改行コードを変換できること」を確認すれば結合テストになります。

関連 issue, PR

#1605
#1504

参考資料

@AppVeyorBot
Copy link

@AppVeyorBot
Copy link

@sonarqubecloud
Copy link

@AppVeyorBot
Copy link

@berryzplus berryzplus marked this pull request as ready for review March 28, 2021 11:40
@berryzplus
Copy link
Contributor Author

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

@berryzplus berryzplus merged commit de751cf into sakura-editor:master Mar 28, 2021
@berryzplus berryzplus deleted the feature/refactoring_of_eeoltype branch March 28, 2021 14:03
extern const EEolType gm_pnEolTypeArr[EOL_TYPE_NUM];

#include "basis/SakuraBasis.h"
constexpr auto EOL_TYPE_NUM = static_cast<size_t>(EEolType::code_max); // 8
Copy link
Contributor

Choose a reason for hiding this comment

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

もとのコメント時点からそうですけど、これ8じゃなくて正確には7ですね、これ

Copy link
Contributor Author

Choose a reason for hiding this comment

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

大昔、 EOL_LFCR という定義があった時代には正しかったようです。
修正しても良かったですが、いったんそのままにしました。

なお、この定数はいずれ削除するつもりです。

@beru beru added the refactoring リファクタリング 【ChangeLog除外】 label Mar 31, 2021
dep5 added a commit to dep5/sakura that referenced this pull request Apr 23, 2021
dep5 added a commit to dep5/sakura that referenced this pull request Apr 23, 2021
dep5 added a commit to dep5/sakura that referenced this pull request Apr 25, 2021
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.

5 participants