final 指定子 と override 指定子の適用 #1090
Conversation
|
✅ Build sakura 1.0.2382 completed (commit c7c2bb08a1 by @beru) |
|
レビュー開始... 119ファイルかぁ・・・ 😭 |
| @@ -42,12 +42,11 @@ class CDlgExec : public CDialog | |||
| SComboBoxItemDeleter m_comboDelCur; | |||
| CRecentCurDir m_cRecentCur; | |||
|
|
|||
| /* オーバーライド? */ | |||
| BOOL OnBnClicked(int wID) override; | ||
| INT_PTR DispatchEvent( HWND hWnd, UINT wMsg, WPARAM wParam, LPARAM lParam ) override; | ||
| BOOL OnSize( WPARAM wParam, LPARAM lParam ) override; | ||
| BOOL OnMove( WPARAM wParam, LPARAM lParam ) override; | ||
| BOOL OnMinMaxInfo( LPARAM lParam ); |
There was a problem hiding this comment.
ここにコメント付けるべきかちょっち悩みどころですね。
overrideなメッセージハンドラの並びに、overrideじゃないハンドラが混じってることをどう捉えるか。
「なんだよ、処理の種類の分類すらできねぇザコが書いてんのかよ！」と嘲笑するのが2ch的デファクトスタンダードですが、ぼくらは先人の遺産を引き継いでく立場なので笑ってる場合じゃないんですよね。
たぶん、記述箇所を移動したほうがよいと思っていますが、別件としたい感じです。
There was a problem hiding this comment.
なんかメッセージハンドラを全部仮想関数にしていたらキリが無い気がしますね。結局派生クラス側でこういう記述が出ているわけだし。
派生クラスでいちいち switch 書きたくないという動機は理解出来ますけど。メッセージマップマクロとかは使いたくなかったんでしょうか？
C# の Windows Forms だとメッセージハンドラは Delegate 登録式ですがインスタンスの各イベント毎にリストのコンテナ持つのは効率が悪すぎるので基底クラスでまとめて持ってるみたいです。
まぁどの記述がどういう観点で好ましいのかは色々あると思うので白黒付けるのは面倒です。。
ここにコメント付けるべきかちょっち悩みどころですね。
overrideなメッセージハンドラの並びに、overrideじゃないハンドラが混じってることをどう捉えるか。
「なんだよ、処理の種類の分類すらできねぇザコが書いてんのかよ！」と嘲笑するのが2ch的デファクトスタンダードですが、ぼくらは先人の遺産を引き継いでく立場なので笑ってる場合じゃないんですよね。
たぶん、記述箇所を移動したほうがよいと思っていますが、別件としたい感じです。
個別に指定出来るなら混ざる記述も当然のように生じるだろうからそこにいちいち神経質になっても…って自分は思います。でも行末コメントがCスタイルのコメントで書かれていると少しイライラするので（ターゲット向けのコンパイラで単一行コメントも使えるのに古い書き方に何故か固執…）自分も似たようなものかも。まぁインデントとかの記述スタイル諸々含めて統一感が取れてないと嫌なのは分かりますが、寛容な心を持たないと人を粛清する暴君になってしまいます。
There was a problem hiding this comment.
#1090 (comment) に書きましたが、このメソッドは override が付いてるのが正しい姿のような気がしてきました。
いずれにしろ別件ですが、どっかで CWnd 派生クラスの整理をしたいなぁ・・とか。
※注：sakuraにもCWnd(=MFCのパクり)が存在していて、ダイアログも CWnd 派生です。
なお、ハンドラマップの独自実装も過去何度か書いてみたことあります 😭
There was a problem hiding this comment.
個人的には WinUI 3.0 に鞍替えしたいです。
There was a problem hiding this comment.
個人的には WinUI 3.0 に鞍替えしたいです。
👍 で。
ただ、現状からイキナリ移行するのは無理だと思っています。
そこに至るまでの課題がいくつもあって、到達目標としては遠すぎるのかな、と。
じゃあ何が課題なんだ！ は「WinUI 3.0に移行したい」で話したほうが良さげです。
| BOOL OnInitDialog(HWND hwndDlg, WPARAM wParam, LPARAM lParam) override; | ||
| BOOL OnBnClicked(int wID) override; | ||
| BOOL OnNotify(WPARAM wParam, LPARAM lParam) override; | ||
| BOOL OnSize( WPARAM wParam, LPARAM lParam ) override; | ||
| BOOL OnMinMaxInfo( LPARAM lParam ); |
There was a problem hiding this comment.
これ、ポップアップダイアログを表すクラスに共通のハンドラですよね。
なんか思い出してきた・・・・。
（処理を分類できてないんじゃなくて、本来あるべき共通の派生元クラスが欠落している
There was a problem hiding this comment.
#1090 (comment)
と同じ内容の話ですね。
There was a problem hiding this comment.
そう、同じ話です。
オブジェクト指向では、「ポップアップで表示するダイアログ」という「もやっとした概念」をクラスとして具象化すべきなんじゃないか？と考えます。まぁ、作成されるのは抽象クラスってやつなんですけども（＾＾； ややこしいｗ
There was a problem hiding this comment.
途中ですが一旦コメント残します。
（修正量が多すぎるのでok/ngを付けて回っています。半分くらいやりましたが一旦ストップ。）
所感ですが、finalを付けると分かりやすくなるもんなんでしょうか？
override修飾は派生元で同じシグニチャのvirtual関数が宣言されていなければエラーになる仕様の修飾子なので、付いていたほうが分かりやすい気がします。
finalはクラスをそれ以上継承できないようにする修飾子ですよね。
個人的には滅多に使わないキーワードだと思っていて、
付けたら分かりやすくなる理由がよく分っていません。
なので、「あれ？これ修正する必要あるの？」と思いながらokの印付けてました。
まぁ、見方によって分かりやすくなるんであれば、変更することに問題はないと思っています。
修正量が多すぎるのはサクラエディタのソースコードの規模が大きいのがそもそもの要因なので難しいところです。PRを分割するのも手ですが…。
本来はクラスを記述する時点で継承させる意図が無い場合にfinal 指定子を付加するべきだと思いますが、昔に書かれたコードなので後付けになってますね。末端クラスに節操なく final を付けたのは自分の好みによるところがあると思います。 final をたくさん付けまくった理由ですが、もしクラスを派生させたいならその時点で基底クラスの final 外せば良いんじゃないかと考えています。final が付いていないという事は 派生クラスが存在するかもと捉える考え方です。とはいえ徹底して付けられる箇所全てに final を付けたわけでは無いです。目 grep しんどい。。 ここから先はOOPよもやま話というかポエムです。 実装継承によって基底クラスの処理を取り込む機能拡張をお手軽に実現出来ます。実装の多重継承（ひし形継承問題が有名）やとても深い継承階層はコードの挙動を把握しにくくなるという事でI/F継承と委譲を使ったやり方を推奨する意見をよく目にすると思います。とはいえ実装継承は便利で多用されているしCRTPとかmixinはtemplateと実装継承を組み合わせたやり方です。 まぁ静的にしろ動的にしろ組み合わせによって空間方向や時間方向に大きく自動展開されたバリエーションを人間が隅々まで把握するのは難しいです。とはいえ高級言語の機能を活用せずに人間が繰り返し同じような記述を行うのも良くありません。 まぁなんだかんだ、罠というかアンチパターンが多いよ、的な方向の話ですが「言語機能の～を使うのは良くない」的な原理主義な考え方は極端な気がするので、あまり思い詰めて結論を急ぐのも良くないと思います。 |
|
final 指定子を付ける付けないは結構好みによるところが多いのでこのPRからは外しても良いと思います。仮想関数に override 指定子を付けるのも継承が絡んでいるので、ついでに色々なクラスに final 指定子も付けてしまいましたが副次的なものです。 |
削んなくてもいいと思いますよ。 |
|
よもやま的な話でいくと、MSVCに古くから付属しているMFCというクラスライブラリが単一継承を前提にしているのが原因で、「MSVCは多重継承をサポートしていない」とか「MSVCでは複数のクラスから派生するクラスを書いてはいけない」とかみたいなデマをよく目にします。 前者について、MFCで多重継承ができないのはCObjectの設計が原因で、MSVCが多重継承できないわけじゃないんですけどね。 |
|
作業を続行しようとしたらコメント数が多すぎてページを開けませんでした。 |
レビュー済みのファイルに |
|
✅ Build sakura 1.0.2383 completed (commit 8e6246f484 by @) |
|
最新chromeで無理でした。 |
| EColorIndexType GetStrategyColor() const override{ return COLORIDX_HEREDOC; } | ||
| CLayoutColorInfo* GetStrategyColorInfo() const override; | ||
| void InitStrategyStatus() override{ m_nCOMMENTEND = 0; } | ||
| void SetStrategyColorInfo(const CLayoutColorInfo*) override; |
There was a problem hiding this comment.
メモ: メソッド宣言に引数が指定されてないコード、結構残ってますね。他にもあった気がします。
使い捨てのvs extension書いて一括対応する話を真面目にやった方がいいのかも。
っていうか、引数名が指定されてなくても動作的な実害はなくて、「ヘッダーに引数指定を書いてはならない」みたいなアフォなコーディング規約も存在しているらしい（＝結構由緒ありそうな公的規約）んで「好みの問題」と言えなくもないんですよね。
※issueの過去ログを検索したら詳細見つかると思います。（手抜き
There was a problem hiding this comment.
っていうか、引数名が指定されてなくても動作的な実害はなくて、「ヘッダーに引数指定を書いてはならない」みたいなアフォなコーディング規約も存在しているらしい（＝結構由緒ありそうな公的規約）んで「好みの問題」と言えなくもないんですよね。
※issueの過去ログを検索したら詳細見つかると思います。（手抜き
多分 #224 (comment) だと思います。
識別子をコメントアウトするのは良い手だと思います。
基本的にプリプロセッサのマクロ名は全て大文字にするというルールを守っていれば問題無いと思いますが、「絶対に大丈夫なんだな？問題が起きたら全ての責任取れるんだな？」と問い詰められたら大人しく引数名をプロトタイプ宣言から削除します。
|
レビューありがとうございます。Merge します。 |
final 指定子 と override 指定子の適用
PR の目的
final 指定子と override 指定子を付加する事でコードを読みやすくするのが目的です。
カテゴリ
PR の背景
final 指定子と override 指定子は C++11 から使えるようになったので比較的新しいC++の機能です。
https://ja.cppreference.com/w/cpp/language/final
https://ja.cppreference.com/w/cpp/language/override
PR のメリット
final 指定子と override 指定子の意味が分かる人にはコードが読みやすくなると思います。
PR のデメリット (トレードオフとかあれば)
final 指定子と override 指定子の意味を学習していない人にはコードが読みにくくなるかもしれません。
PR の影響範囲
動作には影響無い筈です。
不必要と判断した virtual 指定子を削っているので、それにミスがあれば影響が出るかもしれません。
関連チケット