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

共有メモリの基底クラスを変更して破棄のテストを書けるようにする #1582

Merged

Conversation

berryzplus
Copy link
Contributor

PR の目的

共有メモリ管理クラスの基底クラスを変更して、共有メモリに関する検証を行いやすくします。

カテゴリ

  • リファクタリング

PR の背景

#1570util/design_template.h を活用するための変更を行いました。

#1581 とほぼ同件です。

共有メモリは元々「プロセス内に1つのみ」という仕様なので、
変更先の基底クラステンプレートを TSingleInstance<T> とします。

PR のメリット

  • CShareDataが破棄されるときの挙動をテストできるようになります。

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

とくにおもいつきません。

仕様・動作説明

CShareDataの基底クラスを TSingleton から TSingleInstance に変更します。

  • friend 宣言が不要になるので削除します。
  • コンストラクタを protected にする必要がなくなるので public に変更します。
  • CProcessのメンバ変数をCShareData*からCShareDataに変更します。
    ※変数名がハンガリアンなので修正規模が大きく見えます。

ややこしくなるので、変更により可能となるテストケースの追加は行いません。

PR の影響範囲

CShareDataに依存するコードに影響を与える修正です。

テスト内容

修正の主要部分が単体テストのカバー範囲であるため、追加のテストは不要と考えています。

関連 issue, PR

#1570

参考資料

共有メモリへのポインタをm_pShareDataに保持します.このメンバは
公開されていますが,CShareDataによってMap/Unmapされるために
ChareDataの消滅によってポインタm_pShareDataも無効になることに
注意してください.

変更前はサクラエディタ独自「シングルトンっぽいクラス」を基底クラスとしているため、インスタンスを破棄することができません。ここのコメントの大半は現状の挙動に対して「ウソ」になってるのでこれをもって変更の正当性を騙るのはビミョーですが、当初仕様がら離れていく変更でないことの担保にはなると思います。

@berryzplus berryzplus changed the title CShareDataの基底クラスをTSingleInstanceに変更する 共有メモリの基底クラスを変更して破棄のテストを書けるようにする Mar 13, 2021
@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 1 Code Smell

100.0% 100.0% Coverage
0.0% 0.0% Duplication

@AppVeyorBot
Copy link

@berryzplus
Copy link
Contributor Author

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

@berryzplus berryzplus merged commit 33be5af into sakura-editor:master Mar 13, 2021
@berryzplus berryzplus deleted the feature/rebase_csharedata branch March 13, 2021 11:28
@beru beru added the refactoring リファクタリング 【ChangeLog除外】 label Mar 21, 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.

4 participants