履歴管理クラスに関するインクルード文を整理する #1697

ghost · 2021-06-23T05:33:23Z

PR の目的

履歴管理クラス（CRecentインターフェースを実装した各派生クラス）に関するインクルード文を整理します。

カテゴリ

リファクタリング

PR の背景

SonarQubeによる解析結果で、ヘッダファイルのインクルード文に対してCode Smellが検出されていました。

Move all #include directives at the very top of the file, before any code.
- CRecent.h#L86
- CRecentImp.h#L126
non-portable path to file '"CRecentExceptMru.h"'; specified path differs in case from file name on disk
- CRecentExceptMru.cpp#L28

インクルード文の整理を実施して当該指摘の解消を試みます。

PR のメリット

Code Smellが解消されます。
（CRecentインターフェースを利用する側で、）インクルード文からどの履歴管理クラスを利用しているか判断できるようになります。

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

特にありません。

仕様・動作説明

CRecent.h及びCRecentImp.hのファイル末尾にある、派生クラスのインクルード文を削除し、CRecent.hをインクルードしていた各ソースファイルでは代わりに各派生クラスのヘッダファイルをインクルードするようにしました。
作業過程でCRecentの各派生クラスに不必要なインクルード文があったため、併せて削除しました。

PR の影響範囲

履歴データにアクセスする各クラス

テスト内容

ビルドの確認のみです。
なお、MinGWビルドは確認できていませんので、Azure Pipelinesの当該ジョブの成否を確認できるまでDraftとします。

参考資料

CRecent.hに代えて各派生クラスのヘッダファイルをインクルードするようにした。 Signed-off-by: Kohki Akikaze <[email protected]>

Signed-off-by: Kohki Akikaze <[email protected]>

sakura_core/recent/CRecentFolder.h

ghost · 2021-06-23T05:54:42Z

sakura_core/recent/CRecentImp.cpp

+template class CRecentImp<CExcludeFileString, LPCWSTR>;		// CRecentExcludeFile
+template class CRecentImp<CExcludeFolderString, LPCWSTR>;	// CRecentExcludeFolder


CRecentExcludeFile（Grep除外ファイル）・CRecentExcludeFolder（Grep除外フォルダ）のインスタンス化宣言が無かったので追記しました。

テンプレートの明示的インスタンス化はプログラム中で1回までと規定されているため、これらの定義のうち typedef する前の型が重複しているものは実は規格違反だったりします。MinGWでコンパイルを外しているのは GCC の方がこの点を厳密に検査するからかなと推察しました（検証してない）。

重複してしまう定義は削除したほうがいいと思います。下記のような形になりますね。

template class CRecentImp<StaticString<WCHAR, 260>, LPCWSTR>; template class CRecentImp<StaticString<WCHAR, 512>, LPCWSTR>;

可読性・保守性が下がりそうで残念ですけども…。

ちょっと考えましたが、今回は戻しておきます。

可読性・保守性が下がりそうで残念ですけども…。

実はこんな共通定義があります。

sakura/sakura_core/basis/CMyString.h

Lines 42 to 44 in c1568de

//共通型

typedef StaticString<WCHAR,_MAX_PATH> SFilePath;

typedef StaticString<WCHAR, MAX_GREP_PATH> SFilePathLong;

CPathString・CMetaPath・CCurDirString・~~CSearchString・CReplaceString・CTagjumpKeywordString~~はSFilePathと、CGrepFileString・CGrepFolderStringはSFilePathLongと定義内容が同じです。
CExcludeFileString・CExcludeFolderStringの定義で使われる定数MAX_EXCLUDE_PATHはMAX_GREP_PATHと同じ値なので、この二つも現時点ではSFilePathLongと同じと言えます。

この定義を活用して、こう書くこともできますね。

template class CRecentImp<SFilePath, LPCWSTR>; template class CRecentImp<SFilePathLong, LPCWSTR>;

ただ、そうするなら各クラスでもSFilePath/SFilePathLongを利用するようにしたほうがいいのかな、とも思いました。
（typedefは履歴データ構造体側に移せば、長い型名の短縮に使えそうです。）

最も、履歴関係でSFilePath/SFilePathLongを使ってもよいのかはわかりません。

履歴関係でSFilePath/SFilePathLongを使ってもよいのかはわかりません

CSearchString・CReplaceString・CTagjumpKeywordStringは定義こそSFilePathと同じですが、パス文字列ではないので不適切でした😓

sakura_core/recent/CRecentImp.cpp

AppVeyorBot · 2021-06-23T06:07:13Z

✅ Build sakura 1.0.3846 completed (commit e33fa04894 by @kazasaku)

ghost · 2021-06-23T06:12:10Z

Azure PipelinesのMinGWビルドが正常終了したのでDraftは外しておきます。

kengoide · 2021-06-25T15:16:14Z

sakura_core/recent/CRecentImp.cpp

+template class CRecentImp<CExcludeFileString, LPCWSTR>;		// CRecentExcludeFile
+template class CRecentImp<CExcludeFolderString, LPCWSTR>;	// CRecentExcludeFolder


テンプレートの明示的インスタンス化はプログラム中で1回までと規定されているため、これらの定義のうち typedef する前の型が重複しているものは実は規格違反だったりします。MinGWでコンパイルを外しているのは GCC の方がこの点を厳密に検査するからかなと推察しました（検証してない）。

重複してしまう定義は削除したほうがいいと思います。下記のような形になりますね。

template class CRecentImp<StaticString<WCHAR, 260>, LPCWSTR>; template class CRecentImp<StaticString<WCHAR, 512>, LPCWSTR>;

可読性・保守性が下がりそうで残念ですけども…。

Signed-off-by: Kohki Akikaze <[email protected]>

AppVeyorBot · 2021-06-26T03:37:02Z

✅ Build sakura 1.0.3849 completed (commit dc2366f31d by @kazasaku)

sonarcloud · 2021-06-26T04:07:40Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

AppVeyorBot · 2021-06-26T04:07:57Z

✅ Build sakura 1.0.3850 completed (commit 9445b1e552 by @kazasaku)

kengoide

妥当だと思います。

berryzplus

ビルドが通れば問題ないと思います。

細かい指摘入れましたが対応は任意っす。

berryzplus · 2021-06-26T08:42:37Z

sakura_core/dlg/CDlgFavorite.h

+#include "recent/CRecentGrepFile.h"
+#include "recent/CRecentGrepFolder.h"
+#include "recent/CRecentCmd.h"
+#include "recent/CRecentCurDir.h"


レビュー指摘じゃないんですけど、これらのインクルードがここに必要になるクラス設計がマズいように思います。

CRecentの各実装クラスをクラスメンバとして保持しない方が良いということでしょうか。
現状では、以下のクラスがCRecentをメンバに持っています。

CDlgExec

CDlgFavorite

CDlgFind

CDlgGrep

CDlgGrepReplace

CDlgFileData

CDlgReplace

CDlgTagJumpList

CMRUFile

CMRUFolder

CMainToolBar

「CRecentの各実装クラス」の役割が謎なんですよね。

実装から推測されるCRecentクラスの役割

履歴配列へのアクセスを容易にする

履歴配列に格納されるデータ型と文字列ポインタの交換を容易にする

共有メモリへのアクセスを局所化する

たぶん、真面目にクラス設計すると共有メモリへの書き込みアクセスがマルチスレッドを考慮してない件に手を入れる必要があるはずで、その役割を持つのはどのクラスか？と考えるとCRecentが最適な気がするわけです。
　👇
もしコレ（＝CRecentに排他機能を付ける）に踏み切ろうとすると、例示されたウインドウ系クラスが修正対象になります。クラスのメンバにインスタンスを持つということは、ウインドウを表示している間は他スレッドから書き込みできないということで、容易にデッドlロックするアフォな設計になってしまうからです。

メンバにCRecent派生クラスのインスタンスを持たない設計には以下のものがあります。

読み取りアクセスは現状のまま、ロックして書き込む処理を新設する。

ロックが必要なときだけ生成するRAIIクラスを新設する。

排他制御をCRecent側で行い、利用側が意識しないようにした方が使いやすくなりそうですね。
今のところCAppNodeManagerのメンバ関数だけが、CRecentEditNodeの呼び出し直前にロックを掛けていました。

検討してみます。

CDlgOpenFileDataクラスは #1481 のマージ以後CRecentのインスタンスを利用していませんでした。
当該コードの除去をCRecent利用側のリファクタリング案件としてPRを提出しようと思います。

berryzplus · 2021-06-26T08:44:43Z

sakura_core/dlg/CDlgGrep.h

+#include "recent/CRecentGrepFile.h"
+#include "recent/CRecentGrepFolder.h"
+#include "recent/CRecentExcludeFile.h"
+#include "recent/CRecentExcludeFolder.h"


ここも。

berryzplus · 2021-06-26T08:45:01Z

sakura_core/dlg/CDlgGrepReplace.h

@@ -21,6 +21,7 @@ class CDlgGrep;

 #include "dlg/CDialog.h"
 #include "dlg/CDlgGrep.h"
+#include "recent/CRecentReplace.h"


これも。

berryzplus · 2021-06-26T08:45:28Z

sakura_core/dlg/CDlgReplace.h

 #include "util/window.h"
+#include "recent/CRecentReplace.h"
+#include "recent/CRecentSearch.h"


これも。

berryzplus · 2021-06-26T08:50:03Z

sakura_core/recent/CRecentFile.cpp

@@ -26,8 +26,7 @@

 #include "StdAfx.h"
 #include "recent/CRecentFile.h"
-#include <string.h>
-#include "env/DLLSHAREDATA.h"
+#include "config/maxdata.h"


指摘です。ここだけmaxdata.hの参照が実装側になってる気がします。ヘッダ側で参照すべきでは？

ヘッダ側で使われていればヘッダ側に、そうでなければソース側に配置する方針で記述しました。

方針は了解です。

ただ、maxdata.hに定義されているものは定数なので、「インターフェース or 具象クラス」に当てはめた場合のインターフェース側に当たると思います。

インターフェースの参照は、ヘッダで行うべきな気がします。
ま、言ってみてるだけ（≒このコメントは指摘じゃない）ですが。

berryzplus · 2021-06-26T08:50:24Z

sakura_core/recent/CRecentFolder.cpp

@@ -26,8 +26,7 @@

 #include "StdAfx.h"
 #include "CRecentFolder.h"
-#include <string.h>
-#include "env/DLLSHAREDATA.h"
+#include "config/maxdata.h"


あ、ここもやｗ

sakura_core/recent/CRecentFolder.h

berryzplus · 2021-06-26T08:54:10Z

sakura_core/recent/CRecentReplace.cpp

@@ -26,8 +26,7 @@

 #include "StdAfx.h"
 #include "CRecentReplace.h"
-#include <string.h>
-#include "env/DLLSHAREDATA.h"
+#include "config/maxdata.h"


ここもやｗ　（実装側じゃなくヘッダ側に書くべきじゃ・・・

berryzplus · 2021-06-26T08:54:38Z

sakura_core/recent/CRecentTagjumpKeyword.cpp

@@ -26,8 +26,7 @@

 #include "StdAfx.h"
 #include "CRecentTagjumpKeyword.h"
-#include "env/DLLSHAREDATA.h"
-#include <string.h>
+#include "config/maxdata.h"


ここもやｗ　（同上

ghost · 2021-06-29T04:17:14Z

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

CRecentの各派生クラスは今後変更すると思うので、その時にmaxdata.hのインクルード位置を再考します。

ghost · 2021-06-29T14:12:00Z

本文に記載した3件のSonarQubeによる指摘が消えたことを確認しました。

Kohki Akikaze added 2 commits June 22, 2021 14:52

履歴管理クラスのインクルード対象ファイルを変更

2ca3f38

CRecent.hに代えて各派生クラスのヘッダファイルをインクルードするようにした。 Signed-off-by: Kohki Akikaze <[email protected]>

CRecent及びその派生クラスにおけるインクルードファイルの整理

d3cc8ee

Signed-off-by: Kohki Akikaze <[email protected]>

ghost marked this pull request as draft June 23, 2021 05:33

ghost commented Jun 23, 2021

View reviewed changes

sakura_core/recent/CRecentFolder.h Show resolved Hide resolved

ghost commented Jun 23, 2021

View reviewed changes

sakura_core/recent/CRecentImp.cpp Outdated Show resolved Hide resolved

ghost marked this pull request as ready for review June 23, 2021 06:11

beru added the refactoring リファクタリング【ChangeLog除外】 label Jun 23, 2021

ghost changed the title ~~履歴データ管理クラスに関するヘッダーファイルのインクルード文を整理する~~ 履歴管理クラスに関するインクルード文を整理する Jun 25, 2021

kengoide requested changes Jun 25, 2021

View reviewed changes

明示的インスタンス化の追加を撤回

4e06a5c

Signed-off-by: Kohki Akikaze <[email protected]>

kengoide approved these changes Jun 26, 2021

View reviewed changes

berryzplus approved these changes Jun 26, 2021

View reviewed changes

ghost merged commit 9768d7f into sakura-editor:master Jun 29, 2021

ghost deleted the feature/fix_crecent_header_including branch June 29, 2021 14:12

ghost mentioned this pull request Jul 3, 2021

使用されていないCRecentの呼び出し箇所を除去する #1698

Merged

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

履歴管理クラスに関するインクルード文を整理する #1697

履歴管理クラスに関するインクルード文を整理する #1697

ghost commented Jun 23, 2021 •

edited by ghost

Loading

ghost Jun 23, 2021 •

edited by ghost

Loading

kengoide Jun 25, 2021 •

edited

Loading

ghost Jun 26, 2021 •

edited by ghost

Loading

ghost Jun 26, 2021 •

edited by ghost

Loading

AppVeyorBot commented Jun 23, 2021

ghost commented Jun 23, 2021

kengoide Jun 25, 2021 •

edited

Loading

AppVeyorBot commented Jun 26, 2021

sonarcloud bot commented Jun 26, 2021

AppVeyorBot commented Jun 26, 2021

kengoide left a comment

berryzplus left a comment

berryzplus Jun 26, 2021

ghost Jun 27, 2021

berryzplus Jun 27, 2021

berryzplus Jun 27, 2021

ghost Jun 28, 2021

ghost Jun 28, 2021

berryzplus Jun 26, 2021

berryzplus Jun 26, 2021

berryzplus Jun 26, 2021

berryzplus Jun 26, 2021

ghost Jun 27, 2021

berryzplus Jun 27, 2021

berryzplus Jun 26, 2021

berryzplus Jun 26, 2021

berryzplus Jun 26, 2021

ghost commented Jun 29, 2021

ghost commented Jun 29, 2021

		template class CRecentImp<CExcludeFileString, LPCWSTR>; // CRecentExcludeFile
		template class CRecentImp<CExcludeFolderString, LPCWSTR>; // CRecentExcludeFolder

	//共通型
	typedef StaticString<WCHAR,_MAX_PATH> SFilePath;
	typedef StaticString<WCHAR, MAX_GREP_PATH> SFilePathLong;

履歴管理クラスに関するインクルード文を整理する #1697

履歴管理クラスに関するインクルード文を整理する #1697

Conversation

ghost commented Jun 23, 2021 • edited by ghost Loading

PR の目的

カテゴリ

PR の背景

PR のメリット

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

仕様・動作説明

PR の影響範囲

テスト内容

関連 issue, PR

参考資料

ghost Jun 23, 2021 • edited by ghost Loading

Choose a reason for hiding this comment

kengoide Jun 25, 2021 • edited Loading

Choose a reason for hiding this comment

ghost Jun 26, 2021 • edited by ghost Loading

Choose a reason for hiding this comment

ghost Jun 26, 2021 • edited by ghost Loading

Choose a reason for hiding this comment

AppVeyorBot commented Jun 23, 2021

ghost commented Jun 23, 2021

kengoide Jun 25, 2021 • edited Loading

Choose a reason for hiding this comment

AppVeyorBot commented Jun 26, 2021

sonarcloud bot commented Jun 26, 2021

AppVeyorBot commented Jun 26, 2021

kengoide left a comment

Choose a reason for hiding this comment

berryzplus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Jun 29, 2021

ghost commented Jun 29, 2021

ghost commented Jun 23, 2021 •

edited by ghost

Loading

ghost Jun 23, 2021 •

edited by ghost

Loading

kengoide Jun 25, 2021 •

edited

Loading

ghost Jun 26, 2021 •

edited by ghost

Loading

ghost Jun 26, 2021 •

edited by ghost

Loading

kengoide Jun 25, 2021 •

edited

Loading