[WIP] CDataProfileの通常変換からStaticStringを切り離す #1150

berryzplus · 2020-01-13T12:07:24Z

PR の目的

CDataProfileの通常のデータ変換からStaticStringを切り離し、変換対象を分かりやすくします。

カテゴリ

その他
※戻り値の意味を追加したので、リファクタリングではありません。

PR の背景

CDataProfile はデータ変換クラスです。

変換に対応する型は IODataProfile のコメントにリストされている通りです。

sakura/sakura_core/CDataProfile.h

Lines 159 to 169 in 48c5110

    
           	/*! 
        
           	 * 設定値の入出力テンプレート 
        
           	 * 
        
           	 * 標準stringを介して設定値の入出力を行う。 
        
           	 */ 
        
           	template <class T> //T=={bool, int, WORD, wchar_t, char, StringBufferW, StaticString} 
        
           	bool IOProfileData( 
        
           		const WCHAR*			pszSectionName,	//!< [in] セクション名 
        
           		const WCHAR*			pszEntryKey,	//!< [in] エントリ名 
        
           		T&						tEntryValue		//!< [in,out] エントリ値 
        
           	) noexcept

変換に対応する型の中に、他と違う特性を持った型が2つあります。

StaticString<WCHAR, N>　変換対象型の中で唯一、テンプレートパラメータ要求する型です。
StringBufferW　この型は typedef const StringBufferW_ StringBufferW; です（const型です）。

いずれも「文字列」を格納する型なので、ある意味で「無変換」です。
std::wstring とは異なり、バッファにサイズ上限があるので、
無変換でも「変換失敗」が起こりうるため「変換が必要」という奇妙な存在です。

今回は先に、StaticString<WCHAR, N> を通常変換から切り離します。

型がテンプレートパラメータを要求するということは、
「intやboolとは同列に扱えない」ということでもあります。

IOProfileData のオーバーロードテンプレートを用意して、
通常の入出力テンプレートから切り離します。
これにより、通常の入出力テンプレートが対応する変換型は
テンプレートパラメータを要求しない型のみとなります。

PR のメリット

‘StaticString<WCHAR,N>` を切り離すことで通常の入出力テンプレートが分かりやすくなります。
‘StaticString<WCHAR,N>` の変換処理が、他の型変換とは異なることが明確になります。
‘IOProfileData‘ の戻り値で ‘StaticString<WCHAR,N>` を正しく読み込めたか判断できるようになります。
- 変更前は桁溢れ時もtrueを返していました。
- 変更後は桁溢れ時にfalseを返すようになります。
- 変換失敗時（＝桁溢れ）を検出できるようになります。
- 変換失敗時（＝桁溢れ）でも読める分だけのデータを読み込むのは変更前と同じです。

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

とくにありません。

PR の影響範囲

アプリ（＝サクラエディタ）の機能に影響はありません。
修正箇所は、共有メモリに格納する設定値(文字列型)の読み書き処理です。

参考資料

変換対象型の中で唯一、StaticStringだけがテンプレートパラメータを要求する。この特徴をもって「intやboolと同列に扱えない型」と解釈して他と分離しておく。

AppVeyorBot · 2020-01-13T12:55:40Z

✅ Build sakura 1.0.2518 completed (commit c0832983c3 by @berryzplus)

m-tmatma · 2020-01-13T13:53:39Z

sakura_core/CDataProfile.h

+			if( IOProfileData( pszSectionName, pszEntryKey, buf ) ){
+				//StaticString<WCHAR, N>に変換
+				szEntryValue = buf.c_str();
+				ret = buf.length() < _countof2(szEntryValue);


buf.length() と _countof2(szEntryValue) は逆だったりしないですか?
単体テストが欲しい感じですね。

書くとこﾏﾁｶﾞｯﾀ...

buf.length() と _countof2(szEntryValue) は逆だったりしないですか?
単体テストが欲しい感じですね。

あるあるですねｗ
buf.length() ＝代入しようとする文字列の文字列長(NUL終端含まない)
_countof2(szEntryValue) ＝代入先バッファのサイズ(NUL終端含む)
で、「代入する文字列がバッファに収まったらtrue」としたいので合っています。

I/Oクラスなので単体テストを投入するとテスト所要時間が延びます。
CProfile側のチューニング（たぶん、パフォーマンスに難がある）もまだなので
単体テスト投入はギリギリまで先延ばししたいと思っています。

m-tmatma · 2020-01-14T03:47:09Z

これは仕様変更ではなくリファクタリングだと思います

KENCHjp · 2020-01-14T04:20:04Z

内部仕様変更と外部仕様変更（対チェンジログといういみで）とに分けた方がラベル付けは簡単な気もしてますが、ラベルにこだわる必要もあまりない気もしていて、とりあえずラベル付けられてないものを付けている格好です。
冒頭に「リファクタリングではない」との記載があったので、仕様変更にしてみましたが、リファクタリングでもかまいません。

usagisita · 2020-01-14T05:17:33Z

sakura_core/CDataProfile.h

+			//文字列読み込み
+			if( IOProfileData( pszSectionName, pszEntryKey, buf ) ){
+				//StaticString<WCHAR, N>に変換
+				szEntryValue = buf.c_str();


szEntryValue = buf.c_str();って要はwcscpy_sですよね。
これって従来のコメントの通り、_set_invalid_parameter_handlerを設定しないかぎり「後ろを切り詰める」わけでもなくabort/exitしますよね、確か。
その辺はどうする予定なんでしたっけ。

sakura/sakura_core/util/StaticType.h

Lines 106 to 107 in b71888b

void Assign(const CHAR_TYPE* src){ if(!src) m_szData[0]=0; else wcscpy_s(m_szData,_countof(m_szData),src); }

Me& operator = (const CHAR_TYPE* src){ Assign(src); return *this; }

ncpyのtruncateに変えたいです。

ncpyのtruncateに変えたいです。

このコメント残しておくと誤解が生まれそうなので補足します。

ここの意図は、 StaticString<WCHAR,N>::operator = (const WCHAR*) を呼び出した結果で、桁あふれabortが発生しないようにしたいってことです。

そうするために、(wcs)ncpy に文字数 _TRUNCATE を指定するようにしたいと言ってます。

どちらかと言うと、先に「設定ファイル読み取りの失敗を検知できるようにしたい」の issue を立てて CDataProfile のアーキテクチャ変更を進めていくのがよいかな、と思っています。

berryzplus · 2020-01-14T08:19:42Z

せっかくapprove 頂きましたが、単なるリファクタリングに留まるように修正入れたいと思います。たぶん深夜に。

berryzplus · 2020-01-14T14:42:47Z

sakura_core/CDataProfile.h

-	template <int N>
-	void profile_to_value(const wstring& profile, StaticString<WCHAR, N>* value)
-	{
-		wcscpy_s(value->GetBufferPointer(),value->GetBufferCount(),profile.c_str());


う～ん。これ、abortするなぁ・・・。

sakura/sakura_core/CDataProfile.h

Lines 134 to 138 in 8ca0e39

//StringBufferW

void profile_to_value(const wstring& profile, StringBufferW* value)

{

wcscpy_s(value->pData,value->nDataCount,profile.c_str());

}

これもやっぱり abort するなぁ・・・。

#1145 でコメント消したの失敗だったかなぁ・・・。

変更をやめてリファクタリング（＝処理は変えない）にしようと思ったのは、
現状で問題のないコードを問題ある感じにするのはまずい、と思ったからでした。

なんとなく、 wcsncpy_s ＋ _TRUNCATE で「強制切り捨て」にする文化なのかと思っていましたが、思い違いで思い込んでたみたいです。

この辺を踏まえて、追加の修正をしようと思います。

僕もabortで処理するのは最終手段で安全側に倒した結果であるのは理解していますけど、それがテキストエディタに求められるものかと言われると、ちょっと。
要するに起動中にデータが壊れていて設定ファイルに長いデータが一か所でも書き込まれていたら、次回コントロールプロセスが起動中に死亡して、iniを削除しないかぎりエディタとして完全に死ぬということなので。
必要な範囲で、_TRANCATE系で無視するか、エラーを検出してデフォルト値を書き戻すようにするとか、対応がそれぞれあったほうが、より好ましいと思っています。
この辺は、もともとがいい加減だっただけで、新しい方針があるなら、それでいいと思います。

AppVeyorBot · 2020-01-14T15:27:30Z

✅ Build sakura 1.0.2524 completed (commit 236f1dcd0a by @berryzplus)

berryzplus · 2020-01-18T02:47:01Z

IODataProfileのコメント修正を先に済ませたいので一旦保留。

berryzplus · 2020-02-07T01:14:19Z

TODO: issue立てて、全体として何をどうしたいかが分かるようにする。

berryzplus · 2020-02-09T15:24:29Z

やりたいことの最終目標に関しての issue を立てました。

#1188 設定項目がなかったらデフォルト設定を使うようにしたい

で、当座の作業としては、設定が読み込めなかったことを検知する機能をCDataProfileに組み込みたいってことなんで、それが分かる感じのサブissueを立てたいと思っています。

しっくり来るissueタイトルが思いつかないので、今日は諦めて祝日（＝建国記念日）に向けて考える感じっす。

berryzplus · 2020-08-23T06:03:08Z

このPRの変更内容は評価のしようがないと思うので #1363 でやってるプレ・スモークテストの課題が解決してからの導入としたいです。

berryzplus · 2020-09-25T10:11:39Z

モチベーションが尽きたので閉じてしまいます。 #1394

独自型StaticStringを他の変換機構から切り離す。

b71888b

変換対象型の中で唯一、StaticStringだけがテンプレートパラメータを要求する。この特徴をもって「intやboolと同列に扱えない型」と解釈して他と分離しておく。

m-tmatma reviewed Jan 13, 2020

View reviewed changes

m-tmatma previously approved these changes Jan 13, 2020

View reviewed changes

KENCHjp added no-changelog 【ChangeLog除外】 specification change ■仕様変更 labels Jan 13, 2020

usagisita reviewed Jan 14, 2020

View reviewed changes

berryzplus changed the title ~~CDataProfileの通常変換からStaticStringを切り離す~~ [WIP] CDataProfileの通常変換からStaticStringを切り離す Jan 14, 2020

バッファが十分な場合のみ読み込むように修正

fcdd17d

berryzplus dismissed m-tmatma’s stale review via fcdd17d January 14, 2020 14:49

berryzplus changed the title ~~[WIP] CDataProfileの通常変換からStaticStringを切り離す~~ CDataProfileの通常変換からStaticStringを切り離す Jan 14, 2020

berryzplus commented Jan 14, 2020

View reviewed changes

berryzplus changed the title ~~CDataProfileの通常変換からStaticStringを切り離す~~ [WIP] CDataProfileの通常変換からStaticStringを切り離す Jan 18, 2020

berryzplus mentioned this pull request Jan 18, 2020

CDataProfileの「abortします」コメントを復活する #1155

Merged

m-tmatma marked this pull request as draft August 20, 2020 13:25

berryzplus mentioned this pull request Aug 23, 2020

CLogicIntに基本型を指定できるようにする #1322

Closed

berryzplus closed this Sep 25, 2020

berryzplus deleted the feature/split_custom_string branch September 25, 2020 10:11

sanomari mentioned this pull request Feb 21, 2021

設定値読み込みで発生した変換エラーを検出できるようにする #1548

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] CDataProfileの通常変換からStaticStringを切り離す #1150

[WIP] CDataProfileの通常変換からStaticStringを切り離す #1150

berryzplus commented Jan 13, 2020

AppVeyorBot commented Jan 13, 2020

m-tmatma Jan 13, 2020

berryzplus Jan 13, 2020

m-tmatma commented Jan 14, 2020

KENCHjp commented Jan 14, 2020

usagisita Jan 14, 2020

berryzplus Jan 14, 2020

berryzplus Jan 26, 2020

berryzplus commented Jan 14, 2020

berryzplus Jan 14, 2020

usagisita Jan 14, 2020

AppVeyorBot commented Jan 14, 2020

berryzplus commented Jan 18, 2020

berryzplus commented Feb 7, 2020

berryzplus commented Feb 9, 2020

berryzplus commented Aug 23, 2020

berryzplus commented Sep 25, 2020

	/*!
	* 設定値の入出力テンプレート
	*
	* 標準stringを介して設定値の入出力を行う。
	*/
	template <class T> //T=={bool, int, WORD, wchar_t, char, StringBufferW, StaticString}
	bool IOProfileData(
	const WCHAR* pszSectionName, //!< [in] セクション名
	const WCHAR* pszEntryKey, //!< [in] エントリ名
	T& tEntryValue //!< [in,out] エントリ値
	) noexcept

	void Assign(const CHAR_TYPE* src){ if(!src) m_szData[0]=0; else wcscpy_s(m_szData,_countof(m_szData),src); }
	Me& operator = (const CHAR_TYPE* src){ Assign(src); return *this; }

	//StringBufferW
	void profile_to_value(const wstring& profile, StringBufferW* value)
	{
	wcscpy_s(value->pData,value->nDataCount,profile.c_str());
	}

[WIP] CDataProfileの通常変換からStaticStringを切り離す #1150

[WIP] CDataProfileの通常変換からStaticStringを切り離す #1150

Conversation

berryzplus commented Jan 13, 2020

PR の目的

カテゴリ

PR の背景

PR のメリット

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

PR の影響範囲

関連チケット

参考資料

AppVeyorBot commented Jan 13, 2020

m-tmatma Jan 13, 2020

Choose a reason for hiding this comment

berryzplus Jan 13, 2020

Choose a reason for hiding this comment

m-tmatma commented Jan 14, 2020

KENCHjp commented Jan 14, 2020

usagisita Jan 14, 2020

Choose a reason for hiding this comment

berryzplus Jan 14, 2020

Choose a reason for hiding this comment

berryzplus Jan 26, 2020

Choose a reason for hiding this comment

berryzplus commented Jan 14, 2020

berryzplus Jan 14, 2020

Choose a reason for hiding this comment

usagisita Jan 14, 2020

Choose a reason for hiding this comment

AppVeyorBot commented Jan 14, 2020

berryzplus commented Jan 18, 2020

berryzplus commented Feb 7, 2020

berryzplus commented Feb 9, 2020

berryzplus commented Aug 23, 2020

berryzplus commented Sep 25, 2020