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

CFigureにfinalとoverride修飾子を付ける #1417

Merged

Conversation

berryzplus
Copy link
Contributor

PR の目的

final 指定子と override 指定子を付加する事でコードを読みやすくするのが目的です。
過去 #1090 で行った変更と同内容で、前回未適用だった CFigure の派生クラスに対して追加を行います。

カテゴリ

  • リファクタリング

PR の背景

#1405 で CFigure 関連のクラス定義を見て気付きました。

https://ja.cppreference.com/w/cpp/language/final

https://ja.cppreference.com/w/cpp/language/override

PR のメリット

final と override を付けることによるメリットを受けられるようになります。

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

final と override を付けることによりコーディング上の制約が発生します。

仕様・動作説明

仕様・動作に変更はありません。

テスト内容

変更内容が動作に影響しないため、ビルドと簡単な動作確認を行いました。

テスト1

手順
ビルドして起動する。(それだけ。

PR の影響範囲

動作には影響しない変更です。

C++の仕組み上、publicメソッドをprotectedメソッドとしてoverrideすることはできないので、protectedと宣言されていた仮想関数のオーバーライドをpublicに変更しています。

関連 issue, PR

#1090 final 指定子 と override 指定子の適用

参考資料

@AppVeyorBot
Copy link

@usagisita
Copy link
Contributor

僕もちょっと気になっていたんです。これ

@berryzplus
Copy link
Contributor Author

僕もちょっと気になっていたんです。これ

では、レビューをお願いします 😄

最低限評価すべき内容は、以下です。

  • 目的が理解できるか?
  • 目的を許容できるか?
  • 目的の実現方法が適切か?(≒目的を達成できているか?)

Copy link
Contributor

@usagisita usagisita left a comment

Choose a reason for hiding this comment

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

LGTMです。(言ってみたかった)
修正し忘れclassとかも探してみたのの見つからず。
いいと思います。

@berryzplus
Copy link
Contributor Author

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

@berryzplus berryzplus merged commit 8f58ec8 into sakura-editor:master Sep 29, 2020
@berryzplus berryzplus deleted the feature/add_override_to_figures branch September 29, 2020 10:52
@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