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

CDocLine のテストを追加する #1655

Merged
merged 4 commits into from May 3, 2021

Conversation

kengoide
Copy link
Member

@kengoide kengoide commented May 2, 2021

PR の目的

CDocLine に対するテストを追加します。

カテゴリ

  • その他の問題

PR のメリット

  • CDocLine のカバレッジがほぼ100%になります。

仕様・動作説明

テストの障害となる CDocLine::SetEol の共有データ依存を除去する変更を含みます。

PR の影響範囲

CDocLine::SetEol の呼び出し元において自動テストでカバーできない変更が発生しています。以下の機能に影響します。

  • エディタへの編集対象ファイルの読み込み
  • エディタでの文字の挿入・削除
  • GetStrLayoutLength マクロ関数

テスト内容

  • テキストファイルを開き、正しく読み込まれたことを確認する。
  • エディタへの文字を挿入・削除が正しく機能することを確認する。

(GetStrLayoutLength の結果が正しいことも確認するのが望ましいですが、適切なテスト内容が浮かばずにいます。)

@kengoide kengoide force-pushed the feature/tests-for-cdocline branch from fa03529 to 0d79068 Compare May 2, 2021 17:11
@AppVeyorBot
Copy link

Build sakura 1.0.3723 failed (commit 862cf3e27e by @k-kagari)

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.

CSearchAgent::ReplaceData メソッド内に GetDllShareData().m_Common.m_sEdit.m_bEnableExtEol という記述が14か所あります。
結構記述が長いので関数の冒頭辺りで

const auto bEnableExtEol = GetDllShareData().m_Common.m_sEdit.m_bEnableExtEol;

として後はそれを使うようにした方が記述がすっきりするかなと思います。

@kengoide
Copy link
Member Author

kengoide commented May 2, 2021

MinGW Release の失敗は if (this) が最適化で消えてしまう問題でしょうか。

bEnableExtEol

レビューありがとうございます。後ほど反映します。

@AppVeyorBot
Copy link

Build sakura 1.0.3724 completed (commit bb2fe1a37a by @k-kagari)

@kengoide
Copy link
Member Author

kengoide commented May 3, 2021

  • this が null である場合のテストを MinGW ではコンパイルしないようにしました。
  • CSearchAgent.cpp の m_bEnableExtEol 参照に一時変数を導入しました。

#601 C++ の未定義動作によりクラッシュする。(NULL)->MemberFunction();
#1372 MinGW: C++ の未定義動作によるクラッシュを修正する

@sonarcloud
Copy link

sonarcloud bot commented May 3, 2021

@kengoide kengoide marked this pull request as ready for review May 3, 2021 03:56
@AppVeyorBot
Copy link

Build sakura 1.0.3725 completed (commit 64f979a956 by @k-kagari)

Copy link
Contributor

@berryzplus berryzplus left a comment

Choose a reason for hiding this comment

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

テスト不能なクラスCDocLineをテスト可能とする修正として問題なさそうに思います。

@kengoide kengoide merged commit 7e1668d into sakura-editor:master May 3, 2021
@kengoide
Copy link
Member Author

kengoide commented May 3, 2021

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

@kengoide kengoide deleted the feature/tests-for-cdocline branch May 3, 2021 05:45
@berryzplus
Copy link
Contributor

  • this が null である場合のテストを MinGW ではコンパイルしないようにしました。

これは多分「thisがnullである場合に動くコード」を書いていることと、その挙動に依存するコードが「存在すること」が問題だと思います。

JavaやC#では「thisがnullであるオブジェクト」のメソッドを呼べば「ぬるぽ」で落ちます。
C++も同じで「thisがnullであるオブジェクト」のメソッドを呼べばアクセスバイオレーションで落ちます。
ところが、C言語には「オブジェクト」という言語レベルの概念がないので、nullぽアクセスしても落ちないことがあります。

種類 C++ C言語 特徴
virtual関数 × インスタンス必須。
メンバ関数 インスタンス不要。ただし、メンバ変数やvirtual関数を呼ぶにはインスタンス必須。
static関数 × インスタンス不要。
フリー関数 インスタンスとは無縁。グローバル名前空間のフリー関数はグローバル関数と呼ぶ。

サクラエディタは元々Windows APIを多用するC言語プログラムとして書かれていて、C++化した部分とC言語のままな部分とが混在しています。

C言語プログラムでは、配列の終端にNULLを入れて番兵とする疑似コレクションがよく使われるんですけど、この考え方との親和性を持たせるために「thisがnullの場合」を実装してあるんだと思います。

これの対策は NULLオブジェクト の導入なんですけど、導入にはぼちぼち障害があって何もしとらん感じです。

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

4 participants