アウトライン解析データ配列クラスのリファクタリング #1647

beru · 2021-04-27T20:10:29Z

PR の目的

リファクタリングしてコードの可読性を上げるのが目的です。

カテゴリ

リファクタリング

PR の背景

データ要素のポインタ配列ではなく STL の std::vector コンテナを使うように変更しました。
アウトライン解析データ要素の関数名とファイル名のメンバーの型を CNativeW から std::wstring に変更しました。

新しい実装の方がコードがすっきりして可読性が上がります。

PR のメリット

コードの可読性が上がります。

#1644 (comment) でコメントした際には std::vector を使う事でメモリの動的確保の頻度を減らせると考えていましたが、アウトライン解析データ要素の関数名とファイル名のメンバーが文字列型でメモリを動的確保するので、あまり減らないかもしれません。

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

特にないと思います。

仕様・動作説明

リファクタリングなので処理内容には基本的に変更ありません。

PR の影響範囲

アウトライン解析処理

テスト内容

テスト1

手順

何かファイルを開く
アウトライン解析ウィンドウを表示する
リストの要素を選択してキャレットがその行に移動する事を確認
アウトライン解析ウィンドウを閉じて開きなおした時に現在のカーソル行の要素がリスト上で選択されている事を確認

関連 issue, PR

#1644

AppVeyorBot · 2021-04-27T20:50:01Z

✅ Build sakura 1.0.3712 completed (commit e6393eee00 by @beru)

berryzplus

リファクタリングが成立するのか分からんです。

テストを用意しないと、変更前後でふるまいが変わらないことを証明できないです。

「テスト不可コードなので変更前のテストは書けません」はしょうがない気がするんですけど、そういう変更はたぶんリファクタリングじゃないような気がします。

あと、このPRの趣旨は「メモリ確保を削減する」だと思うんですが、
cNative.AppendNativeData(wstring.cstr())はマズいと思います。

暗黙的に cNative.AppendNativeData(CNativeW(wstring.cstr())) になるからです。

berryzplus · 2021-05-18T13:26:41Z

sakura_core/dlg/CDlgJump.cpp

@@ -257,12 +257,12 @@ void CDlgJump::SetData( void )
 			if( m_pShareData->m_bLineNumIsCRLF_ForJump ){	/* 行番号の表示 false=折り返し単位／true=改行単位 */
 				auto_sprintf( szText, LS(STR_DLGJUMP_PSLQL),
 					cFuncInfoArr.GetAt( i )->m_nFuncLineCRLF,
-					cFuncInfoArr.GetAt( i )->m_cmemFuncName.GetStringPtr()
+					cFuncInfoArr.GetAt( i )->m_cmemFuncName.c_str()


なんか、レビューする意欲がわかないのですが、こう書けるようにしたら今よりマシになるかと思います。

Suggested change

cFuncInfoArr.GetAt( i )->m_cmemFuncName.c_str()

cFuncInfoArr.GetAt( i ).GetFuncName()

GetAt(n) の戻り値をポインタから参照に変えれば、nullptr考慮が不要になります。

LPCWSTR を返すうんこgetterを作れば、外部に影響を与えずにメンバ型を変更できるようになります。

もし CFuncInfoArr::GetAt の戻り値の型をポインタから参照に変えるとなると、引数の nIdx が範囲外だった場合に例外を送る必要が出てくるかもしれないと思いました。下記のような感じでしょうか？

/* 0<=の指定番号のデータを返す */ /* データがない場合は例外を送出する */ CFuncInfo& CFuncInfoArr::GetAt( int nIdx ) { if( nIdx < 0 || nIdx >= m_cFuncInfoArr.size() ){ throw std::out_of_range(__func__); } return m_cFuncInfoArr[nIdx]; }

なお CFuncInfoArr::GetAt の戻り値が nullptr じゃないかの確認は CDlgFuncList::CompareFunc_Asc では行っていますが他では行っていないですね。それで問題が無いのかというとちょっと心配な箇所もありますがおそらく大丈夫なんでしょう。

さて戻り値をポインタから参照に変えれば、nullptr の考慮は不要にはなると思いますが try catch を使った例外処理の記述が不十分だと回復できなくてアプリが落ちてしまいます。ただそれは現状の実装でも nullptr 経由でメモアクセスした場合には segmentation fault になるのでそういう意味ではいっしょかな？

ただ CFuncInfoArr::GetAt の呼び出しの外で try catch を書く手間を考えたら、CFuncInfoArr::GetNum を呼んで事前に問題が無いindexなのかどうか調べる方が良い気がしてきました。

実装するならnullオブジェクトだと思います。
例外にすると、説明してもらったような問題がでるので。

LPCWSTR CFuncInfo::GetFuncName() const;

を追加する方が確かにコードの見栄えが良くなりますね。

あと、このPRの趣旨は「メモリ確保を削減する」だと思うんですが、
cNative.AppendNativeData(wstring.cstr())はマズいと思います。

暗黙的に cNative.AppendNativeData(CNativeW(wstring.cstr())) になるからです。

これはその通りですね。ご指摘ありがとうございます。何も考えずに元のコードのまま AppendNativeData を使うようにしていました。後で AppendString を使うように修正を行います。

リファクタリングが成立するのか分からんです。

テストを用意しないと、変更前後でふるまいが変わらないことを証明できないです。

「テスト不可コードなので変更前のテストは書けません」はしょうがない気がするんですけど、そういう変更はたぶんリファクタリングじゃないような気がします。

リファクタリングする際はテストを書いて変更前と処理結果が一致する事を確認していきながら進めた方が良いよってマーティン・ファウラーの本でも書かれてますね。手抜きしてテストは極力書いていませんが、ちゃんとテストしないと不具合がぽこぽこ出てしまうので反省してなるべく追加したいと思います。

なおリファクタリングの定義については個々人の心の中にあると思うので、別にテストを書かなくてもリファクタリングだとは思ってますが、他の人がそれを受け入れてくれるかは別ですね…。

GetAt の戻り値を参照に変える変更を行いましたがそれに付随してかなり差分は増えました。
目視でのコードレビューはかなり大変そうです。

なお単体テストを用意しても変更前後で動作が変わらない事の証明にはなりません。一定の条件での確認にはなると思います。

単体テストが用意されてないとテストが自動再現出来ないしテストのエビデンスも残りにくいので、テスト追加しないと変更しちゃ駄目だよという意見はまぁもっともなんですけど、なんか気軽に変更しづらいなぁと思います。

「リファクタリング」については、設計のリファクタリングをどう扱うかに迷いがあるためにこんな話し方になっています。単体テストを用意して「変更前後で挙動に差はありません」を担保するスタイルだと、「インターフェースが変」なのを対策するリファクタリングができないです。サクラエディタで「やったほうが良さそう」なネタとしてはインターフェース変更を伴うリファクタリングが多くて、しかもそういう変更は変更前がテスト不能である場合が多い気がします。

なお単体テストを用意しても変更前後で動作が変わらない事の証明にはなりません。一定の条件での確認にはなると思います。

その通りだと思います。

「できるだけ単体テストを用意しようぜ！」と言ってる目的の一つは、
「テストが書けない」って事実に気付けるようにすることです。

検証不能なロジックは、検証コードを書いても結局検証できないっす。

書けないなりにテストを書いてみて、そのテストじゃ動作検証が十分に行えないと分かったならそれは対策すべきだと思うんですが、たぶん絶対まだそんな段階には達していないと思うので検証が行えてないのは一旦スルーでよいような気がします。ノリ的に「とりあえず努力はそとこうぜ」です。

このPRでは色々なファイルを変更していますが、CDialog を継承した CDlgFuncList と CDlgJump についてはGUIなので単体テストで試験しにくそうなので手動で動作確認を念入りに行うようにします。

アウトライン解析データ配列のクラスである CFuncInfoArr についてはまだテストコードが無いようですが、単体テストが行いやすい内容だと思うので後で追加を行います。

berryzplus · 2021-05-18T13:28:13Z

sakura_core/outline/CDlgFuncList.cpp

@@ -127,7 +127,7 @@ int CALLBACK CDlgFuncList::CompareFunc_Asc( LPARAM lParam1, LPARAM lParam2, LPAR
 	}
 	//	Apr. 23, 2005 genta 行番号を左端へ
 	if( FL_COL_NAME == pcDlgFuncList->m_nSortCol){	/* 名前でソート */
-		return wmemicmp( pcFuncInfo1->m_cmemFuncName.GetStringPtr(), pcFuncInfo2->m_cmemFuncName.GetStringPtr() );
+		return wmemicmp( pcFuncInfo1->m_cmemFuncName.c_str(), pcFuncInfo2->m_cmemFuncName.c_str() );


_wcsicmpのほうが良いのでは？と思います。

_wcsicmp の方が良いと思った理由について書いてもらえると参考になるので助かります。ロケール設定が絡むとこで自分にはどちらの方が良いのかの判断がしにくいです。wmemicmp を使った比較処理に何か問題があるのなら変えるべきだと思いますが、別にそういうわけではないならそのままでも良いんじゃないでしょうか？

wchar単位のバイナリデータを比較したいわけでなく、wchar文字列を比較する処理に見えたからです。ロケールについて考慮していませんでしたが、そこは動作環境のデフォルトロケールで良いように思います。

wmemicmp の実装では skr_towlower を呼び出してwchar単位で比較していますが、これだと目的に合致しないという事でしょうか？サロゲートペアとかが関係しているのでしょうか？

まぁどちらにしろこのPRではここの挙動は変えたくないのでここについては変更しません。

sakura_core/outline/CDlgFuncList.cpp

berryzplus · 2021-05-18T13:33:20Z

sakura_core/outline/CDlgFuncList.cpp

@@ -600,7 +600,7 @@ void CDlgFuncList::SetData()
 			ListView_SetItem( hwndList, &item);

 			item.mask = LVIF_TEXT;
-			item.pszText = const_cast<WCHAR*>(pcFuncInfo->m_cmemFuncName.GetStringPtr());
+			item.pszText = const_cast<WCHAR*>(pcFuncInfo->m_cmemFuncName.c_str());


分かっててやってると思いますが、一般的に const_cast の使用は不適切です。

Suggested change

item.pszText = const_cast<WCHAR*>(pcFuncInfo->m_cmemFuncName.c_str());

item.pszText = pcFuncInfo->m_cmemFuncName.data();

としたらイケる場合があります（const std::wstringな場合は無理ですが。）

分かっててやってると思いますが、一般的に const_cast の使用は不適切です。

まず元のコードで const_cast 使ってるのでそれを何も考えずに踏襲しています。あと代入先の LVITEMW::pszText の型が LPWSTR (WCHAR*) なのでキャストが入るのは仕方が無い気がします。

berryzplusさんがどういう考えでこういう事を書いているかというのは自分にはよくわかりません。C++ではconst修飾子を付ける事で型システムがコンパイル時に検査してくれるから、それから逸脱するコードを限りなく避けるべきだという考えですか？自分はサクラエディタの既存のコードを使う延長線上でそこまで厳密で安全なプログラミングを追求したいわけではないです。表面上の const_cast を無くす事で何がそんなにおいしい結果に繋がるのかさっぱりわかりません。

あと data メソッドを使ってこのケースでは無理な事を確認しました。const CFuncInfo* pcFuncInfo; だからです。

windows APIの型定義が原因で発生するキャストですよね。必要があるからキャストしてるわけなので、キャスト自体に問題はないと思っています。「合法」という言い方は嫌いなんですが、合法なキャストです。

ここで指摘したかったのは、不適切なキャストをせざるを得ない実装をあちこち書くの？ということで、「自称」StdApi.cppなどの独自定義APIラッパーを作成したほうがいいんじゃない？ということでした。

ここで指摘したかったのは、不適切なキャストをせざるを得ない実装をあちこち書くの？ということで、「自称」StdApi.cppなどの独自定義APIラッパーを作成したほうがいいんじゃない？ということでした。

一般的に const_cast の使用は不適切です。 というコメントの真意が 独自定義APIラッパーを作成したほうがいいんじゃない？ だったんですか。それは随分と遠回しに感じますがご出身は京都ですか？

煩雑な記述を隔離してすっきりさせたいという気持ちは分かりますが、もしそういう事をするなら別のPRで全体的に対処するやり方でないと片付かない気がします。

StdApi.cppが独自定義APIラッパーなのかといわれて見てみましたが内容を見てもしっくりきません。CommonControl.h や StdControl.h の方であればまぁラッパーぽい気がします。

あとこの件に関して調べてみましたが ListView_SetItemText マクロを使う事でコードの記述を少しすっきりさせる事は出来ると思います。ただPRの差分量は増えますし（既に多いのでもはや野となれ山となれかもしれませんが）ここの記述だけ更新してもサクラエディタのソースコード全体に対して対処するわけではないので統一感が出ないと思います。

sakura_core/outline/CDlgFuncList.cpp

berryzplus · 2021-05-18T13:46:07Z

sakura_core/outline/CDlgFuncList.cpp

@@ -1402,7 +1402,7 @@ void CDlgFuncList::SetTree(bool tagjump, bool nolabel)
 		for( int i = 0; i < nFuncInfoArrNum; i++ ){
 			const CFuncInfo* pcFuncInfo = m_pcFuncInfoArr->GetAt(i);
 			if( pcFuncInfo->IsAddClipText() ){
-				nBuffLen += pcFuncInfo->m_cmemFuncName.GetStringLength() + pcFuncInfo->m_nDepth * 2;
+				nBuffLen += pcFuncInfo->m_cmemFuncName.size() + pcFuncInfo->m_nDepth * 2;


名前のサイズ＋深さ×２

という値に意味のある名前が付かないでしょうか？

ここだけ設計よりな話です。

名前の話は置いておいて、そもそも何でこういう処理が記述されているのかについてですが、クリップボードコピー用テキストのバッファの最大サイズを事前に求める事で、後でクリップボードコピー用テキストを作成する処理で m_cmemClipText.AppendNativeData を呼び出す際にバッファサイズが足りなかった場合の再確保＆コピー等が行われるのを避けているんだと思います。

ただこのバッファ最大サイズの事前計算の記述が結構長いのでちょっと嫌になります。更にそもそもアウトライン解析ウィンドウのメニューのコピーを選択してクリップボードにコピーをするという操作を常に行うとは限らないのに、毎回この処理が走るので無駄骨感があります。初回のコピー操作時にクリップボード用のテキストを作るようにした方が良いと思いますが、分けて処理を記述するのが面倒くさかったんでしょうかね。。

さて名前に関しては、https://yosshin4004.github.io/famibe/develop/index.html によると「変数名はなるべく 1 文字に」と書かれているのでそれに従ってあえて縛りプレイをするのもジョークになるので良いと思います。C++コンパイラ使う時点で変数名がある程度長くても問題は無いですが1文字にすれば考える手間を省けますね！

・・・。

CDlgFuncList::SetTreeに関係ない処理かもっす。
とナナメ読みしました。
必要な時に計算したほうが分かりやすい気がしました。
（今回やらなくていいと思います。）

名前についての話は、通じてないように思います。

nBuffLen += pcFuncInfo->m_cmemFuncName.size() + pcFuncInfo->m_nDepth * 2;
　👇
nBuffLen += pcFuncInfo->GetNanikaNoNagasa();

とできませんか？と言ってみただけっす（対応不要っす。

これに関してはローカル変数の名前で表現するべきかもしれません。

どんな名前にするべきかを考えるためにクリップボードコピー用テキストを作成する処理の記述を少し読んでみましたが、結構記述が長くてここで事前に m_cmemClipText.AllocStringBuffer を呼び出して確保するバッファサイズと一致する（もしくは足りる）サイズのテキスト量を後の処理で追加しているのかというと…なんだか記述が一致しなさ過ぎて読んでもよくわかりません。

バッファの最大サイズを事前に求めて reserve する事で無駄なメモリの再確保は減らせる事は確かですが、正確に最大サイズを求めるとなると同じ記述を繰り返し書くことになるのでコード量が増えてしまいます。

あとクリップボードコピー用テキストを作成する処理において別のローカル変数 CNativeW text を用意してそこに1行分の文字列を設定する記述があるので、メモリの動的確保は無駄に毎回行われてしまっています。

前のコメントでも書きましたが、このメソッドでクリップボードコピー用テキストを作成する処理を実行しているのがそもそも良くないと思います。あとテキストの最大サイズの事前計算の記述がありますが、コードの記述が増えてしまうわりに効果は微妙だと思うのでやらない方が良いと思います。

長文になりましたが要約するとやっても無意味っす。

sakura_core/outline/CDlgFuncList.cpp

berryzplus · 2021-05-18T13:47:46Z

sakura_core/outline/CDlgFuncList.cpp

 				if( bFileJump ){
 					nLineTo = m_cFuncInfo->m_nFuncLineCRLF;
 					nColTo = m_cFuncInfo->m_nFuncColCRLF;
 					// 別のファイルへジャンプ
 					CMyPoint poCaret; // TagJumpSubも1開始
 					poCaret.x = nColTo;
 					poCaret.y = nLineTo;
-					bFileJumpSelf = TagJumpTimer(m_cFuncInfo->m_cmemFileName.GetStringPtr(), poCaret, bCheckAutoClose);
+					bFileJumpSelf = TagJumpTimer(m_cFuncInfo->m_cmemFileName.c_str(), poCaret, bCheckAutoClose);


独自定義関数 TagJumpTimer の第一引数を std::wstring_view にしたら .c_str() 付ける必要がなくなるっす。

std::wstring_view を使うのは良い考えですね。是非そういう風にしたいと思います。

berryzplus · 2021-05-18T13:49:27Z

sakura_core/outline/CDlgFuncList.cpp

-		if( (pcFuncInfo->m_cmemFileName.GetStringPtr() && m_pcFuncInfoArr->m_szFilePath[0]) ){
-			if( 0 != wmemicmp( pcFuncInfo->m_cmemFileName.GetStringPtr(), m_pcFuncInfoArr->m_szFilePath.c_str() ) ){
+		if( (!pcFuncInfo->m_cmemFileName.empty() && m_pcFuncInfoArr->m_szFilePath[0]) ){
+			if( 0 != wmemicmp( pcFuncInfo->m_cmemFileName.c_str(), m_pcFuncInfoArr->m_szFilePath.c_str() ) ){


_wcsicmpを使ったほうが意図が明確になる気がします。

#1647 (comment) にコメントしました。

AppVeyorBot · 2021-05-19T01:34:18Z

✅ Build sakura 1.0.3769 completed (commit 1db1b47cad by @beru)

AppVeyorBot · 2021-05-19T02:11:14Z

✅ Build sakura 1.0.3770 completed (commit 83ad28c113 by @beru)

AppVeyorBot · 2021-05-19T02:51:29Z

✅ Build sakura 1.0.3771 completed (commit d0053c2e41 by @beru)

AppVeyorBot · 2021-05-19T03:28:59Z

✅ Build sakura 1.0.3772 completed (commit 67d4d6e64c by @beru)

AppVeyorBot · 2021-05-19T04:05:25Z

✅ Build sakura 1.0.3773 completed (commit c10602696c by @beru)

AppVeyorBot · 2021-05-19T04:44:07Z

✅ Build sakura 1.0.3774 completed (commit c4f5d1c0b3 by @beru)

suconbu · 2021-05-19T16:48:15Z

sakura_core/outline/CFuncInfo.h

+		std::wstring	m_cmemFuncName;	/*!< 関数名 */
+		std::wstring	m_cmemFileName;	/*!< ファイル名 */


std::wstring 型になりましたので変数名から cmem をなくして良いと思いました。

接頭辞の cmem を無くして代わりに str を付けました。付けなくても Name があるから文字列型だと分かる気もしますが何かつけておかないとハンガリアン記法の禁断症状が出てしまいます。

suconbu · 2021-05-19T16:54:12Z

sakura_core/outline/CFuncInfoArr.h

@@ -23,6 +23,7 @@ class CFuncInfo;
 #include "util/design_template.h"
 #include "basis/SakuraBasis.h"
 #include "basis/CMyString.h"
+#include "outline/CFuncInfo.h"


これが追加されましたので20行目にある CFuncInfo の前方宣言はもういらなさそうです。

指摘に対応しました。

AppVeyorBot · 2021-05-21T01:34:14Z

✅ Build sakura 1.0.3778 completed (commit 5e8090a340 by @beru)

suconbu · 2021-05-21T03:58:39Z

sakura_core/outline/CFuncInfoArr.h

 	void AppendData( CLogicInt nFuncLineCRLF, CLayoutInt nFuncLineLAYOUT, const WCHAR* pszFuncName,
 					 int nInfo, int nDepth = 0 );	/* 配列の最後にデータを追加する 2002.04.01 YAZAKI 深さ導入*/
 	void AppendData( CLogicInt nLogicLine, CLogicInt nLogicCol, CLayoutInt nLayoutLine, CLayoutInt nLayoutCol, const WCHAR*, const WCHAR*, int, int nDepth = 0 );	/* 配列の最後にデータを追加する 2010.03.01 syat 桁導入*/
-	int	GetNum( void ){	return m_nFuncInfoArrNum; }	/* 配列要素数を返す */
+	size_t GetNum( void ) const {	return m_cFuncInfoArr.size(); }	/* 配列要素数を返す */


GetNum を呼び出している箇所にて芋づる式に int -> size_t 修正が必要になってしまうため、今回の対応では int のままが良いのではと思いました。

確かに64bitビルドで警告が出るので戻り値の型を size_t に変えると呼び出し元を変更する必要が生じてしまいますね。
GetAt メソッドの引数は int のままなのと要素数も int で十分なはずなので元に戻しておきます。

この int か size_t かついては #1564 (comment) で話題になっていたのを思い出し、悩ましいところだと思いました。

#1679 (comment) でも言及されている UBSan(UndefinedBehaviorSanitizer) で問題が検出できるのかもしれないですがVC++ではまだ使えないし過信しない方が良さそうです。

#1564 (comment) で k-kagariさんが

Google C++ Style Guide は size_t を historical accident とまで呼んでいたりします。

と書かれていたので検索してみたところ https://google.github.io/styleguide/cppguide.html#Integer_Types の最後に該当する文が書かれていましたので引用します。

On Unsigned Integers

Unsigned integers are good for representing bitfields and modular arithmetic. Because of historical accident, the C++ standard also uses unsigned integers to represent the size of containers - many members of the standards body believe this to be a mistake, but it is effectively impossible to fix at this point. The fact that unsigned arithmetic doesn't model the behavior of a simple integer, but is instead defined by the standard to model modular arithmetic (wrapping around on overflow/underflow), means that a significant class of bugs cannot be diagnosed by the compiler. In other cases, the defined behavior impedes optimization.

That said, mixing signedness of integer types is responsible for an equally large class of problems. The best advice we can provide: try to use iterators and containers rather than pointers and sizes, try not to mix signedness, and try to avoid unsigned types (except for representing bitfields or modular arithmetic). Do not use an unsigned type merely to assert that a variable is non-negative.

最後の文の Do not use an unsigned type merely to assert that a variable is non-negative. というのは自分が過去に犯した過ちなのできちんと覚えておこうと思いました。。

Google C++ Style Guide は size_t を historical accident とまで呼んでいたりします。

size_t は型名に u の文字が含まれないのもあってか、ぱっと見、符号なしには見えないのが危険ポイントな気がしますね。
うっかりラップアラウンドさせて暴走事故を起こさないよう気を付けたいと思います。

// size_t GetNum() for( size_t i = 0; i < GetNum() - 1; ++i ){ ...

suconbu · 2021-05-21T05:31:33Z

#1644 (comment) でコメントした際には std::vector を使う事でメモリの動的確保の頻度を減らせると考えていましたが、アウトライン解析データ要素の関数名とファイル名のメンバーが文字列型でメモリを動的確保するので、あまり減らないかもしれません。

CFuncInfoArr::AppendData を多数呼び出す CDocOutline::MakeTopicList_txt へ CRunningTimer を使った計測処理を入れ、反映前後での処理時間を比較してみました。
ばらつきあるものの 10% 程度は速くなったようです。
(CDlgFuncList::SetData が圧倒的に時間がかかる (8秒程度) ので誤差のような処理時間ですけど……)

CDocOutline::MakeTopicList_txt の処理時間 (ms)：

	PR 反映前 (`2e01db2` + 計測処理)	PR 反映後 (`204010e` + 計測処理)
1回目	65.703	49.697
2回目	60.225	54.167
3回目	58.287	72.487
4回目	63.585	57.254
5回目	63.681	56.639

最大・最小を除いた平均	62.497	56.02

ビルド環境：Release/Win32
テストデータ：#1398 に添付されている test.zip の中の test_100k.txt
計測時の操作：test_100k.txt 読み込み後、アウトライン解析を表示し、F11 キーを 2 回押した時のログを取得

usagisita · 2021-05-22T05:18:36Z

sakura_core/outline/CDlgFuncList.cpp

 			CNativeW text;
 			if( tagjump ){
-				const WCHAR* pszFileName = pcFuncInfo->m_cmemFileName.GetStringPtr();
+				const WCHAR* pszFileName = cFuncInfo.m_strFileName.c_str();
 				if( pszFileName == NULL ){


std::wstringに変更したことによりNULLにならなくなったので、何らかの修正が必要そうです。
空文字列チェックでいいとは思います。""とNULLが区別されるべき場所ではなさそうです。

文字列クラスは空文字で初期化するので nullptr は取らなくなるんですね。仕様を把握してませんでした。
empty メソッドで判別するように修正しておきます。

beru · 2021-05-22T05:30:51Z

CFuncInfoArr::AppendData を多数呼び出す CDocOutline::MakeTopicList_txt へ CRunningTimer を使った計測処理を入れ、反映前後での処理時間を比較してみました。
ばらつきあるものの 10% 程度は速くなったようです。
(CDlgFuncList::SetData が圧倒的に時間がかかる (8秒程度) ので誤差のような処理時間ですけど……)

そういえば処理時間の事を考えていませんでした。CDlgFuncList::SetData の処理時間は #1648 がマージされて高速化したので Rebase を行うようにします。

データ要素のポインタ配列ではなく　std::vector　を使うように変更アウトライン解析データ要素の関数名とファイル名のメンバーの型を CNativeW から std::wstring に変更

Co-authored-by: berryzplus <berryzplus@gmail.com>

sonarcloud · 2021-05-22T13:49:39Z

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
0 Security Hotspots
28 Code Smells

24.4% Coverage
11.3% Duplication

AppVeyorBot · 2021-05-22T14:50:46Z

✅ Build sakura 1.0.3788 completed (commit 552f9a1c46 by @beru)

AppVeyorBot · 2021-05-22T15:43:59Z

✅ Build sakura 1.0.3789 completed (commit 4ecbaf48fb by @beru)

AppendNativeDataの件は直ってそうなので外します。

usagisita · 2021-07-26T03:01:25Z

sakura_core/outline/CDlgFuncList.cpp

 {
 	CEditView* pcView = reinterpret_cast<CEditView*>(m_lParam);

 	// ファイルを開いていない場合は自分で開く
 	if( pcView->GetDocument()->IsAcceptLoad() ){
-		std::wstring strFile = pFile;
-		pcView->GetCommander().Command_FILEOPEN( strFile.c_str(), CODE_AUTODETECT, CAppMode::getInstance()->IsViewMode(), NULL );
+		pcView->GetCommander().Command_FILEOPEN( strFile.data(), CODE_AUTODETECT, CAppMode::getInstance()->IsViewMode(), NULL );


strFileを引数に変更していますが、wstring_viewになったので実データの生存期間が明確ではありません。
例えば引数がアウトラインデータ由来でFILEOPENのときに先行してアウトラインの破棄などが「もし」動作すると、Command_FILEOPEN内部で引数をコピーしてくれていない場合は、実データをロストしてしまう可能性があります。(ダングリングポインタ)
FILEOPEN側の責任ともいえるので、万が一の場合ではありますが、ほんの少し心配です。

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

berryzplus previously requested changes May 18, 2021

View reviewed changes

suconbu reviewed May 20, 2021

View reviewed changes

suconbu reviewed May 21, 2021

View reviewed changes

usagisita reviewed May 22, 2021

View reviewed changes

beru and others added 8 commits May 22, 2021 22:15

Update sakura_core/outline/CDlgFuncList.cpp

02671cf

Co-authored-by: berryzplus <berryzplus@gmail.com>

Update sakura_core/outline/CDlgFuncList.cpp

93d50f1

Co-authored-by: berryzplus <berryzplus@gmail.com>

Update sakura_core/outline/CDlgFuncList.cpp

a4b1fef

Co-authored-by: berryzplus <berryzplus@gmail.com>

Update sakura_core/outline/CDlgFuncList.cpp

aa00f70

Co-authored-by: berryzplus <berryzplus@gmail.com>

Update sakura_core/outline/CDlgFuncList.cpp

f50282f

Co-authored-by: berryzplus <berryzplus@gmail.com>

レビュー指摘対応

7ba76cf

レビュー指摘対応

d7d0025

beru force-pushed the refactoring_CFuncInfoArr branch from 04cd642 to d7d0025 Compare May 22, 2021 13:32

beru marked this pull request as draft May 27, 2021 15:21

usagisita reviewed Jul 26, 2021

View reviewed changes

	cFuncInfoArr.GetAt( i )->m_cmemFuncName.c_str()
	cFuncInfoArr.GetAt( i ).GetFuncName()

	item.pszText = const_cast<WCHAR*>(pcFuncInfo->m_cmemFuncName.c_str());
	item.pszText = pcFuncInfo->m_cmemFuncName.data();

		std::wstring m_cmemFuncName; /!< 関数名 /
		std::wstring m_cmemFileName; /!< ファイル名 /

アウトライン解析 データ配列クラスのリファクタリング #1647

Are you sure you want to change the base?

アウトライン解析 データ配列クラスのリファクタリング #1647

Conversation

beru commented Apr 27, 2021 • edited

PR の目的

カテゴリ

PR の背景

PR のメリット

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

仕様・動作説明

PR の影響範囲

テスト内容

テスト1

関連 issue, PR

AppVeyorBot commented Apr 27, 2021

berryzplus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

beru May 18, 2021 • edited

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

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

AppVeyorBot commented May 19, 2021

AppVeyorBot commented May 19, 2021

AppVeyorBot commented May 19, 2021

AppVeyorBot commented May 19, 2021

AppVeyorBot commented May 19, 2021

AppVeyorBot commented May 19, 2021

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

AppVeyorBot commented May 21, 2021

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

suconbu commented May 21, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

beru commented May 22, 2021

sonarcloud bot commented May 22, 2021

AppVeyorBot commented May 22, 2021

AppVeyorBot commented May 22, 2021

Choose a reason for hiding this comment

アウトライン解析データ配列クラスのリファクタリング #1647

アウトライン解析データ配列クラスのリファクタリング #1647

beru commented Apr 27, 2021 •

edited

beru May 18, 2021 •

edited