Code modernize C++17, refactor and minor optimize #1860

GermanAizek · 2022-07-18T15:23:47Z

PR の目的

C++コードの標準化、C++17への近似、コードリファクタリング、コンパイラのマイナーコード最適化。

カテゴリ

リファクタリング

PR の背景

PR のメリット

目標は、コードの可読性を向上させ、標準化し、コンパイラのマイクロレベルで最適化することです

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

特にないと思います。

仕様・動作説明

PR の影響範囲

テスト内容

テスト1

手順

参考資料

CH 🇨🇳 :
https://www.cnblogs.com/narjaja/p/10039509.html [emplace vs insert]

EN 🇺🇸 :
https://cplusplus.com/forum/general/122033/ [emplace vs insert]
https://stackoverflow.com/questions/14788261/c-stdvector-emplace-vs-insert [emplace vs insert]
https://stackoverflow.com/questions/20828907/the-new-syntax-default-in-c11 ['= default;' constructor]
https://en.cppreference.com/w/cpp/language/default_constructor ['= default;' constructor]
https://www.modernescpp.com/index.php/memory-and-performance-overhead-of-smart-pointer [std::make_unique (C++17) vs new unique_ptr]
https://stackoverflow.com/questions/22571202/differences-between-stdmake-unique-and-stdunique-ptr-with-new [std::make_unique (C++17) vs new unique_ptr]
https://stackoverflow.com/questions/37689228/difference-between-str-clear-and-str [str.clear() vs str = ""]
https://stackoverflow.com/questions/483337/c-is-string-empty-always-equivalent-to-string [str.empty() vs str == ""]

AppVeyorBot · 2022-07-18T15:57:10Z

✅ Build sakura 1.0.4169 completed (commit ab5af49e0a by @GermanAizek)

GermanAizek

@beru, @berryzplus 私はコードとフィードバックのあなたの改訂を楽しみにしています

berryzplus · 2022-07-19T04:19:58Z

修正提案ありがとうございます。
以下コメントに対して思うところあって confused アイコンを付けました。

私はコードとフィードバックのあなたの改訂を楽しみにしています

フィードバックが期待されるときは Pull Request する前に issue を立てて相談するほうが好ましいです。

修正方針には概ね合意です。
ただ、何点か追加の対応が必要な修正が含まれています。

Pull Request を部分的に approve することはできないため、
たくさんの修正をまとめて Pull Request すると
一部の「ダメな修正」のために「何の問題もない修正」をマージできない結果になります。

修正量と修正内容（≒カテゴリ）が多いので、
このまま指摘を書き始めると大変なことになりそうです。

あと SonarCloud の指摘は適切と思います。

Security Hotspot E 3 Security Hotspots
Code Smell A 3 Code Smells

「ネストレベルが多いのをリファクタリングせよ」以外は「対応してほしいです。」といつも指摘しています。

sakura_core/CDicMgr.cpp

sakura_core/CGrepEnumFileBase.h

sakura_core/basis/CMySize.h

sakura_core/cmd/CViewCommander_Grep.cpp

sakura_core/dlg/CDlgPluginOption.cpp

berryzplus · 2022-07-19T06:29:22Z

sakura_core/dlg/CDlgPluginOption.cpp

@@ -159,7 +158,7 @@ void CDlgPluginOption::SetData( void )
 		}

 		if (cOpt->GetType() == OPTION_TYPE_BOOL) {
-			wcscpy( buf, sValue == wstring( L"0") || sValue == wstring( L"") ? BOOL_DISP_FALSE : BOOL_DISP_TRUE );
+			wcscpy( buf, sValue == wstring( L"0") || sValue == wstring() ? BOOL_DISP_FALSE : BOOL_DISP_TRUE );


Suggested change

wcscpy( buf, sValue == wstring( L"0") || sValue == wstring() ? BOOL_DISP_FALSE : BOOL_DISP_TRUE );

wcscpy_s( buf, sValue == L"0"s || sValue.empty() ? BOOL_DISP_FALSE : BOOL_DISP_TRUE );

@berryzplus, this not work on MSVC compiler

sValue == L"0"s

C++ user-defined literal operator not found

L"0"s requires using statement as follows.

using namespace std::string_literals;

I made additional commit 1c71b64 to fix issue.

sakura_core/extmodule/CBregexp.cpp

sakura_core/macro/CSMacroMgr.h

sakura_core/plugin/CPlugin.cpp

sakura_core/typeprop/CImpExpManager.cpp

sakura_core/util/design_template.h

berryzplus · 2022-07-19T06:49:04Z

sakura_core/util/file.cpp

@@ -851,6 +850,7 @@ void GetExistPathW( wchar_t *po , const wchar_t *pi )

 	dl = GetExistPath_NO_DriveLetter;	/*「ドライブレターが無い」にしておく*/
 	if( *(po+1)==L':' && WCODE::IsAZ(*po) ){	/* 先頭にドライブレターがある。そのドライブが有効かどうか判定する */
+		wchar_t	drv[4] = L"_:\\";


Suggested change

wchar_t drv[4] = L"_:\\";

constexpr auto& drv = L"_:\\";

@berryzplus,
This drv should not be constexpr const wchar_t (&drv)[4] because its first element is changed below the line

drv[0] = *po;

sakura_core/view/CEditView.cpp

Signed-off-by: lainon <GermanAizek@yandex.ru>

AppVeyorBot · 2022-07-19T10:23:06Z

❌ Build sakura 1.0.4170 failed (commit 4656df30e5 by @berryzplus)

usagisita · 2022-07-19T16:15:34Z

sakura_core/CGrepEnumFileBase.h

@@ -115,8 +115,9 @@ class CGrepEnumFileBase {
 	int Enumerates( LPCWSTR lpBaseFolder, VGrepEnumKeys& vecKeys, CGrepEnumOptions& option, CGrepEnumFileBase* pExceptItems = NULL ){
 		int found = 0;

+		const auto cchBaseFolder = lpBaseFolder ? wcsnlen_s(lpBaseFolder, _MAX_PATH - 1) : 0;


(日本語ですみません)
Grep周りは、GUIでもフォルダパスなどにMAX_PATHの倍の長さ(約510文字)を入力できます。
またコマンドライン経由でGrepを起動すれば、もっと長いパスも受け付けます。
実際に、\\?\のUNC形式などを使用すると、Windows10の長いパス対応でなくても、MAX_PATHを超えるパスでGrepすることが可能なはずです。近年の新規コードで制限が増えていなければですが。
ここでMAX_PATH制限をすることは、適切だとは思えません。

テキトーな上限値4096 - 1を設定してみました。

usagisita · 2022-07-19T16:17:54Z

参考
http://sakura.qp.land.to/?Request%2F443

AppVeyorBot · 2022-07-19T17:37:47Z

✅ Build sakura 1.0.4171 completed (commit a51e13ac61 by @berryzplus)

berryzplus · 2022-07-19T17:44:50Z

自分的には LGTM ですが、コードをpushしてしまったので他メンバーにレビューをお願いしたいです。

制限事項

同じ理屈で修正できるコードで対応してないものが残ってますが、放置します。

AppVeyorBot · 2022-07-19T18:11:56Z

✅ Build sakura 1.0.4172 completed (commit afcbf5a1d3 by @berryzplus)

sakura_core/view/CEditView.cpp

Shiftを押さなかった場合にスクロールしなくなってしまっていたのを修正。

sonarcloud · 2022-07-20T01:03:31Z

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
0 Security Hotspots
7 Code Smells

12.2% Coverage
0.0% Duplication

AppVeyorBot · 2022-07-20T01:05:44Z

✅ Build sakura 1.0.4173 completed (commit c0acf24e3f by @berryzplus)

beru · 2022-07-20T16:30:31Z

sakura_core/CGrepEnumKeys.h

-	auto GetExcludeFiles() const ->  std::list<decltype(m_vecExceptFileKeys)::value_type> {
-		std::list<decltype(m_vecExceptFileKeys)::value_type> excludeFiles;
+	auto GetExcludeFiles() const ->  std::vector<decltype(m_vecExceptFileKeys)::value_type> {
+		std::vector<decltype(m_vecExceptFileKeys)::value_type> excludeFiles;
 		const auto& fileKeys = m_vecExceptFileKeys;
 		excludeFiles.insert( excludeFiles.cend(), fileKeys.cbegin(), fileKeys.cend() );
 		const auto& absFileKeys = m_vecExceptAbsFileKeys;


auto excludeFiles = m_vecExceptFileKeys; excludeFiles.reserve(excludeFiles.size() + m_vecExceptFileKeys.size()); excludeFiles.insert( excludeFiles.cend(), m_vecExceptFileKeys.cbegin(), m_vecExceptFileKeys.cend() ); return excludeFiles;

こういう記述でも良いと思います。

コンテナの要素を全部追加する関数とかは標準では無いんですよね。
よくやる操作ですが糖衣構文が無いしちょっと記述が長くなりがちですね。
https://stackoverflow.com/questions/2551775/appending-a-vector-to-a-vector

素っ頓狂なことを言うかもですがstd::copyが標準関数にあたるんじゃないかと思いました。

std::copy( m_vecExceptFileKeys.cbegin(), m_vecExceptFileKeys.cend(), std::back_inserter(excludeFiles) );

全体をコピーする場合にわざわざ範囲指定したくないですが、範囲指定が必要な場合もあるので指定するI/Fの処理が標準であるんでしょうね。

記述量を減らしたいならそそもC++を使うべきでないという気もしますが、書く手間が掛かる言語だなぁと思いました。

実は、コピー自体不要になってるかもです・・・。

std::vector を std::list に変換するメンバー関数だったんですが、
戻り値を vector に変えちゃったので、
詰め替えのためのコピーは要らなくなってる気がします。

beru · 2022-07-20T17:11:26Z

sakura_core/outline/CDlgFuncList.cpp

@@ -762,7 +762,7 @@ void CDlgFuncList::SetData()

 bool CDlgFuncList::GetTreeFileFullName(HWND hwndTree, HTREEITEM target, std::wstring* pPath, int* pnItem)
 {
-	*pPath = L"";
+	(*pPath).clear();


pPath->clear();

の方が個人的にはすっきりとした書き方だと思いますが、まぁどうでもいい事ですね。

むしろ、個人的にはシグニチャ変えたいっす（笑

std::wstring* pPath,
　↓
std::wstring& path,

beru · 2022-07-20T17:18:23Z

sakura_core/plugin/CPlugin.h

@@ -185,22 +185,20 @@ class CPluginOption
 	//コンストラクタ
 public:
 	CPluginOption( CPlugin* parent, wstring sLabel, wstring sSection, wstring sKey, wstring sType, wstring sSelects, wstring sDefaultVal, int index) 


ここは引数の型を wstring から const wstring& に変えて呼び出し時のインスタンスのコピーを減らすのが良いと思います。

std::move で引数のインスタンスの所有権をメンバー変数に移す事でもメモリ確保は減らせるかもしれませんが、 CPlugin::ReadPluginDefOption で CPluginOption のインスタンス作成時にコンストラクタを呼び出す際にそもそもローカル変数の不要なコピーを発生させない方が良いのかなと。

作法的には std::wstring よりも const std::wstring& が好ましく、
さらに const std::wstring& よりも std::wstring_view が好ましいようです。
C言語の文字列関数との混用が多いので _view では部分文字列に配慮する必要があって修正がややこしくなることがめんどくさいですが。

beru

差分を確認した限りでは問題無さそうに思えます。
変更点が多いので変更箇所を全て動作確認するのは大変そうで行ってません。

berryzplus · 2022-07-21T07:41:05Z

細かいことを言い出すと大変そうなので、いったんマージしてしまいます。
なにか問題を見つけたら都度部分的に対応していく進め方にできたらな、と思います。

Code modernize C++17, refactor and minor optimize

c491dd4

GermanAizek commented Jul 18, 2022

View reviewed changes